Bug 27952 – cssgrammar.cpp fails to compile with RVCT compiler

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

The error message is the following:

armcc [...] cssgrammar.cpp

"css\CSSGrammar.y", line 743: Error:  #171: invalid type conversion
          if (equalIgnoringCase(static_cast<const String&>(str), "from"))
                                ^
"css\CSSGrammar.y", line 745: Error:  #171: invalid type conversion
          else if (equalIgnoringCase(static_cast<const String&>(str), "to"))
                                     ^
cssgrammar.cpp: 0 warnings, 2 errors

Few notes:
 - cssgrammar.cpp is a generated file from CSSGrammar.y.
 - CSSParserString (the type for str above) offers a cast operator to String but RVCT does not seems to be able to understand it.

[Laszlo Gombos]

Created attachment 34033 [details]
proposed patch.

Invoke the cast to String operator by hand.

[Eric Seidel (no email)]

Comment on attachment 34033 [details]
proposed patch.

This is going to copy the string.  that's an extra malloc during parsing, and likely a slowdown. :(

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 34033 [details])
> This is going to copy the string.  that's an extra malloc during parsing, and
> likely a slowdown. :(

The old code already copies the string. This is bad. The best fix that fixes both compiling and improves performance is to create a charactersEqualIgnoringCase function that works with a character pointer and length. This would be like the other "characters" functions in PlatformString.h.

We could also overload equalIgnoringCase for CSSParserString; that would just be a convenience that would make it slightly easier to read.

Code to do this is already present in StringImpl.cpp; it should be relatively straightforward to refactor it.

In the mean time I actually see nothing wrong with Laszlo's initial patch, except that there is a stray space after "selector". But I'd love it if we could get rid of the stray memory allocation here.

[Laszlo Gombos]

Created attachment 34201 [details]
second try.

Darin, thanks for the detailed recommendation. As you suggested I get rid of the stray allocation by working directly with characters.

[Darin Adler]

Comment on attachment 34201 [details]
second try.

I think this change is OK.

I'd prefer that function declarations that don't need argument names don't have them. And the one for equalIgnoringCase uses "a" and "b" even though those don't add any information.

I'd prefer it if all the string-like functions that operate on runs of characters were grouped together. At this point, the others are in PlatformString.h, but these new ones are in StringImpl.h.

I'd prefer it if the functions that work with characters were as consistent as possible. The ones in PlatformString.h use size_t for length, but this one uses unsigned. The ones in PlatformString.h use "characters" as a prefix on the function name, but this one uses overloading instead. The ones in PlatformString.h put the UChar* and length first, and other arguments afterward, but these new ones do not.

Finally, I think tis function should be more clear on how it handles the const char*. If that's intended to be a null-terminated C string, then we need to check for the null character, because otherwise if the UChar contains a 0 in just the right place we could walk off the end of the passed in const char*. Alternatively, if we want to pass in the length then I think we'd need a second length argument for the const char*. I don't think the function definition makes it clear how it's intended to be used.

But I don't think any of these concerns should prevent you from landing this. I just think they would be things to improve in the future.

[Simon Hausmann]

Fix landed in r46846

/me agrees with Darins comments

[Adam Barth]

This patch claims to have been landed.