27952 2009-08-03 13:53:38 -0700 cssgrammar.cpp fails to compile with RVCT compiler 2009-08-07 01:02:25 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 27065 1 laszlo.gombos laszlo.gombos abarth darin hausmann norbert.leser oldest_to_newest 136770 0 laszlo.gombos 2009-08-03 13:53:38 -0700 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. 136883 1 34033 laszlo.gombos 2009-08-03 18:56:53 -0700 Created attachment 34033 proposed patch. Invoke the cast to String operator by hand. 137006 2 34033 eric 2009-08-04 08:56:31 -0700 Comment on attachment 34033 proposed patch. This is going to copy the string. that's an extra malloc during parsing, and likely a slowdown. :( 137047 3 darin 2009-08-04 10:24:42 -0700 (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. 137532 4 34201 laszlo.gombos 2009-08-05 23:38:10 -0700 Created attachment 34201 second try. Darin, thanks for the detailed recommendation. As you suggested I get rid of the stray allocation by working directly with characters. 137575 5 34201 darin 2009-08-06 08:16:07 -0700 Comment on attachment 34201 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. 137582 6 hausmann 2009-08-06 08:59:32 -0700 Fix landed in r46846 /me agrees with Darins comments 137819 7 abarth 2009-08-07 01:02:25 -0700 This patch claims to have been landed. 34033 2009-08-03 18:56:53 -0700 2009-08-05 23:38:10 -0700 proposed patch. pending_patch_27952.txt text/plain 1307 laszlo.gombos SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0Njc0MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDktMDgtMDMgIExhc3psbyBHb21ib3MgIDxsYXN6bG8uMS5nb21i b3NAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIGNzc2dyYW1tYXIuY3BwIGZhaWxzIHRvIGNvbXBpbGUgd2l0aCBSVkNUIGNvbXBpbGVy CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzk1Mgor CisgICAgICAgICogY3NzL0NTU0dyYW1tYXIueTogSW52b2tlIHRoZSBjYXN0IHRvIFN0cmluZyBv cGVyYXRvciBieSBoYW5kCisKIDIwMDktMDgtMDMgIEplcmVteSBPcmxvdyAgPGpvcmxvd0BjaHJv bWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gRmlzaGVyLgpJbmRleDogV2Vi Q29yZS9jc3MvQ1NTR3JhbW1hci55Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvY3NzL0NTU0dyYW1t YXIueQkocmV2aXNpb24gNDY3MjYpCisrKyBXZWJDb3JlL2Nzcy9DU1NHcmFtbWFyLnkJKHdvcmtp bmcgY29weSkKQEAgLTc0MCw5ICs3NDAsMTAgQEAga2V5OgogICAgIHwgSURFTlQgewogICAgICAg ICAkJC5pZCA9IDA7ICQkLmlzSW50ID0gZmFsc2U7ICQkLnVuaXQgPSBDU1NQcmltaXRpdmVWYWx1 ZTo6Q1NTX05VTUJFUjsKICAgICAgICAgQ1NTUGFyc2VyU3RyaW5nJiBzdHIgPSAkMTsKLSAgICAg ICAgaWYgKGVxdWFsSWdub3JpbmdDYXNlKHN0YXRpY19jYXN0PGNvbnN0IFN0cmluZyY+KHN0ciks ICJmcm9tIikpCisgICAgICAgIFN0cmluZyBzZWxlY3RvciAoc3RyLmNoYXJhY3RlcnMsIHN0ci5s ZW5ndGgpOworICAgICAgICBpZiAoZXF1YWxJZ25vcmluZ0Nhc2Uoc2VsZWN0b3IsICJmcm9tIikp CiAgICAgICAgICAgICAkJC5mVmFsdWUgPSAwOwotICAgICAgICBlbHNlIGlmIChlcXVhbElnbm9y aW5nQ2FzZShzdGF0aWNfY2FzdDxjb25zdCBTdHJpbmcmPihzdHIpLCAidG8iKSkKKyAgICAgICAg ZWxzZSBpZiAoZXF1YWxJZ25vcmluZ0Nhc2Uoc2VsZWN0b3IsICJ0byIpKQogICAgICAgICAgICAg JCQuZlZhbHVlID0gMTAwOwogICAgICAgICBlbHNlCiAgICAgICAgICAgICBZWUVSUk9SOwo= 34201 2009-08-05 23:38:10 -0700 2009-08-06 08:16:07 -0700 second try. patch_27952_2.txt text/plain 2874 laszlo.gombos SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NjgzNSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMDktMDgtMDUgIExhc3psbyBHb21ib3MgIDxsYXN6bG8uMS5nb21i b3NAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIGNzc2dyYW1tYXIuY3BwIGZhaWxzIHRvIGNvbXBpbGUgd2l0aCBSVkNUIGNvbXBpbGVy CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzk1Mgor CisgICAgICAgICogY3NzL0NTU0dyYW1tYXIueTogRG8gbm90IGNvbnZlcnQgdG8gU3RyaW5nIHRv IGdldCByaWQgb2YgdGhlIHN0cmF5CisgICAgICAgIG1lbW9yeSBhbGxvY2F0aW9uIAorCisgICAg ICAgICogcGxhdGZvcm0vdGV4dC9TdHJpbmdJbXBsLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmVx dWFsSWdub3JpbmdDYXNlKToKKyAgICAgICAgKiBwbGF0Zm9ybS90ZXh0L1N0cmluZ0ltcGwuaDog CisgICAgICAgIChXZWJDb3JlOjplcXVhbElnbm9yaW5nQ2FzZSk6IEFkZCBjaGFyYWN0ZXJzRXF1 YWxJZ25vcmluZ0Nhc2UKKyAgICAgICAgICBmdW5jdGlvbiB0aGF0IHdvcmtzIHdpdGggYSBjaGFy YWN0ZXIgcG9pbnRlciBhbmQgbGVuZ3RoCisKIDIwMDktMDgtMDUgIERpbWl0cmkgR2xhemtvdiAg PGRnbGF6a292QGNocm9taXVtLm9yZz4KIAogICAgICAgICBVbnJldmlld2VkLCBidWlsZCBmaXgu CkluZGV4OiBXZWJDb3JlL2Nzcy9DU1NHcmFtbWFyLnkKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9j c3MvQ1NTR3JhbW1hci55CShyZXZpc2lvbiA0NjcyNikKKysrIFdlYkNvcmUvY3NzL0NTU0dyYW1t YXIueQkod29ya2luZyBjb3B5KQpAQCAtNzQwLDkgKzc0MCw5IEBAIGtleToKICAgICB8IElERU5U IHsKICAgICAgICAgJCQuaWQgPSAwOyAkJC5pc0ludCA9IGZhbHNlOyAkJC51bml0ID0gQ1NTUHJp bWl0aXZlVmFsdWU6OkNTU19OVU1CRVI7CiAgICAgICAgIENTU1BhcnNlclN0cmluZyYgc3RyID0g JDE7Ci0gICAgICAgIGlmIChlcXVhbElnbm9yaW5nQ2FzZShzdGF0aWNfY2FzdDxjb25zdCBTdHJp bmcmPihzdHIpLCAiZnJvbSIpKQorICAgICAgICBpZiAoZXF1YWxJZ25vcmluZ0Nhc2UoImZyb20i LCBzdHIuY2hhcmFjdGVycywgc3RyLmxlbmd0aCkpCiAgICAgICAgICAgICAkJC5mVmFsdWUgPSAw OwotICAgICAgICBlbHNlIGlmIChlcXVhbElnbm9yaW5nQ2FzZShzdGF0aWNfY2FzdDxjb25zdCBT dHJpbmcmPihzdHIpLCAidG8iKSkKKyAgICAgICAgZWxzZSBpZiAoZXF1YWxJZ25vcmluZ0Nhc2Uo InRvIiwgc3RyLmNoYXJhY3RlcnMsIHN0ci5sZW5ndGgpKQogICAgICAgICAgICAgJCQuZlZhbHVl ID0gMTAwOwogICAgICAgICBlbHNlCiAgICAgICAgICAgICBZWUVSUk9SOwpJbmRleDogV2ViQ29y ZS9wbGF0Zm9ybS90ZXh0L1N0cmluZ0ltcGwuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxh dGZvcm0vdGV4dC9TdHJpbmdJbXBsLmNwcAkocmV2aXNpb24gNDY3MjYpCisrKyBXZWJDb3JlL3Bs YXRmb3JtL3RleHQvU3RyaW5nSW1wbC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUzOSw3ICs1Mzks NyBAQCBzdGF0aWMgYm9vbCBlcXVhbChjb25zdCBVQ2hhciogYSwgY29uc3QgCiAgICAgcmV0dXJu IHRydWU7CiB9CiAKLXN0YXRpYyBib29sIGVxdWFsSWdub3JpbmdDYXNlKGNvbnN0IFVDaGFyKiBh LCBjb25zdCBjaGFyKiBiLCBpbnQgbGVuZ3RoKQorYm9vbCBlcXVhbElnbm9yaW5nQ2FzZShjb25z dCBVQ2hhciogYSwgY29uc3QgY2hhciogYiwgdW5zaWduZWQgbGVuZ3RoKQogewogICAgIEFTU0VS VChsZW5ndGggPj0gMCk7CiAgICAgd2hpbGUgKGxlbmd0aC0tKSB7CkluZGV4OiBXZWJDb3JlL3Bs YXRmb3JtL3RleHQvU3RyaW5nSW1wbC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0v dGV4dC9TdHJpbmdJbXBsLmgJKHJldmlzaW9uIDQ2NzI2KQorKysgV2ViQ29yZS9wbGF0Zm9ybS90 ZXh0L1N0cmluZ0ltcGwuaAkod29ya2luZyBjb3B5KQpAQCAtMjIwLDYgKzIyMCw4IEBAIGlubGlu ZSBib29sIGVxdWFsKGNvbnN0IGNoYXIqIGEsIFN0cmluZ0kKIGJvb2wgZXF1YWxJZ25vcmluZ0Nh c2UoU3RyaW5nSW1wbCosIFN0cmluZ0ltcGwqKTsKIGJvb2wgZXF1YWxJZ25vcmluZ0Nhc2UoU3Ry aW5nSW1wbCosIGNvbnN0IGNoYXIqKTsKIGlubGluZSBib29sIGVxdWFsSWdub3JpbmdDYXNlKGNv bnN0IGNoYXIqIGEsIFN0cmluZ0ltcGwqIGIpIHsgcmV0dXJuIGVxdWFsSWdub3JpbmdDYXNlKGIs IGEpOyB9Citib29sIGVxdWFsSWdub3JpbmdDYXNlKGNvbnN0IFVDaGFyKiBhLCBjb25zdCBjaGFy KiBiLCB1bnNpZ25lZCBsZW5ndGgpOworaW5saW5lIGJvb2wgZXF1YWxJZ25vcmluZ0Nhc2UoY29u c3QgY2hhciogYSwgY29uc3QgVUNoYXIqIGIsIHVuc2lnbmVkIGxlbmd0aCkgeyByZXR1cm4gZXF1 YWxJZ25vcmluZ0Nhc2UoYiwgYSwgbGVuZ3RoKTsgfQogCiAvLyBHb2xkZW4gcmF0aW8gLSBhcmJp dHJhcnkgc3RhcnQgdmFsdWUgdG8gYXZvaWQgbWFwcGluZyBhbGwgMCdzIHRvIGFsbCAwJ3MKIC8v IG9yIGFueXRoaW5nIGxpa2UgdGhhdC4K