Bug 27952 - cssgrammar.cpp fails to compile with RVCT compiler
Summary: cssgrammar.cpp fails to compile with RVCT compiler
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Laszlo Gombos
Depends on:
Blocks: 27065
  Show dependency treegraph
Reported: 2009-08-03 13:53 PDT by Laszlo Gombos
Modified: 2009-08-07 01:02 PDT (History)
4 users (show)

See Also:

proposed patch. (1.28 KB, patch)
2009-08-03 18:56 PDT, Laszlo Gombos
eric: review-
Details | Formatted Diff | Diff
second try. (2.81 KB, patch)
2009-08-05 23:38 PDT, Laszlo Gombos
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-08-03 13:53:38 PDT
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.
Comment 1 Laszlo Gombos 2009-08-03 18:56:53 PDT
Created attachment 34033 [details]
proposed patch.

Invoke the cast to String operator by hand.
Comment 2 Eric Seidel (no email) 2009-08-04 08:56:31 PDT
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. :(
Comment 3 Darin Adler 2009-08-04 10:24:42 PDT
(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.
Comment 4 Laszlo Gombos 2009-08-05 23:38:10 PDT
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.
Comment 5 Darin Adler 2009-08-06 08:16:07 PDT
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.
Comment 6 Simon Hausmann 2009-08-06 08:59:32 PDT
Fix landed in r46846

/me agrees with Darins comments
Comment 7 Adam Barth 2009-08-07 01:02:25 PDT
This patch claims to have been landed.