67851 2011-09-09 09:18:34 -0700 WTFString.h should include template specialization for HashTraits<String> 2011-09-14 07:19:58 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 husky webkit-unassigned darin dglazkov husky jamesr kbr mjs sam steveblock webkit.review.bot oldest_to_newest 464826 0 husky 2011-09-09 09:18:34 -0700 WTFString.h should include template specialization for HashTraits<String> 464830 1 106879 husky 2011-09-09 09:23:33 -0700 Created attachment 106879 Patch 464833 2 husky 2011-09-09 09:27:51 -0700 See also https://bugs.webkit.org/show_bug.cgi?id=67848 which has more detail on where I ran into this problem. 465354 3 sam 2011-09-10 17:21:00 -0700 I am not sure I understand why this is necessary. Lots of file include <wtf/text/StringHash.h>. 465562 4 husky 2011-09-12 04:14:09 -0700 This code doesn't compile and I think it should: #include <wtf/HashSet.h> #include <wtf/text/WTFString.h> class ScoobyDoo { HashSet<String> scrappy; }; #include <wtf/text/StringHash.h> Here's the error I get: wtf/text/StringHash.h: At global scope: wtf/text/StringHash.h:182: error: specialization of ‘WTF::HashTraits<WTF::String>’ after instantiation wtf/text/StringHash.h:182: error: redefinition of ‘struct WTF::HashTraits<WTF::String>’ wtf/HashTraits.h:57: error: previous definition of ‘struct WTF::HashTraits<WTF::String>’ In file included from wtf/text/StringImpl.h:33, from wtf/text/WTFString.h:28, This can occur if one header file includes HashSet.h and String.h and declares a HashSet<String> (perfectly reasonable on its own) and then a later header file includes StringHash.h. I hit this in a unit test I'm working on, so it's not theoretical. If you don't include StringHash.h, it compiles, but I think it picks up the wrong HashTraits (because it's missing the template specialization). We're doing this a lot, so it seems important. 465644 5 sam 2011-09-12 09:55:31 -0700 (In reply to comment #4) > This code doesn't compile and I think it should: > > #include <wtf/HashSet.h> > #include <wtf/text/WTFString.h> > > class ScoobyDoo { > HashSet<String> scrappy; > }; > > #include <wtf/text/StringHash.h> > > Here's the error I get: > > wtf/text/StringHash.h: At global scope: > wtf/text/StringHash.h:182: error: specialization of ‘WTF::HashTraits<WTF::String>’ after instantiation > wtf/text/StringHash.h:182: error: redefinition of ‘struct WTF::HashTraits<WTF::String>’ > wtf/HashTraits.h:57: error: previous definition of ‘struct WTF::HashTraits<WTF::String>’ > In file included from wtf/text/StringImpl.h:33, > from wtf/text/WTFString.h:28, > > This can occur if one header file includes HashSet.h and String.h and declares a HashSet<String> (perfectly reasonable on its own) and then a later header file includes StringHash.h. I hit this in a unit test I'm working on, so it's not theoretical. I don't think it is perfectly reasonable. I believe you are supposed to #include <wtf/text/StringHash.h> if you want to use a HashSet<String>. Perhaps we should make it easier to ensure you are doing this right, but I am not sure #including <wtf/HashTraits.h> everywhere we #include <wtf/text/WTFString.h> is the right way to go (it might be though.) 465708 6 husky 2011-09-12 11:52:07 -0700 Including StringHash.h directly is the workaround I use in that other patch (https://bugs.webkit.org/show_bug.cgi?id=67848) so it sounds like we should go ahead and submit that one. If we could figure out a way to force a compile error if you forget to include StringHash.h, that would be great. It seems a little risky right now because you don't even get a warning if you do the wrong thing. 465898 7 darin 2011-09-12 16:01:15 -0700 (In reply to comment #6) > If we could figure out a way to force a compile error if you forget to include StringHash.h, that would be great. It seems a little risky right now because you don't even get a warning if you do the wrong thing. Yes, agreed. 466192 8 husky 2011-09-13 01:36:34 -0700 I assume the problem with my original suggestion is header bloat and compilation time -- is that right? As an alternative to pulling in HashTraits if you #include WTFString.h, how about we flip it around, and pull in String if you #include HashTraits? That would add some header bloat to all HashMap/HashSet users, but there must be fewer of them than there are String users. 466203 9 107153 husky 2011-09-13 02:16:56 -0700 Created attachment 107153 Patch 466204 10 husky 2011-09-13 02:18:52 -0700 Just uploaded an alternate fix. This moves HashTraits<String> into HashTraits.h. We can actually forward-declare String so there isn't any additional bloat. 466205 11 webkit.review.bot 2011-09-13 02:18:56 -0700 Attachment 107153 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/HashTraits.h:31: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 466225 12 107153 webkit.review.bot 2011-09-13 03:12:23 -0700 Comment on attachment 107153 Patch Attachment 107153 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9660075 New failing tests: svg/W3C-SVG-1.1/animate-elem-12-t.svg svg/W3C-SVG-1.1/animate-elem-04-t.svg svg/W3C-SVG-1.1/animate-elem-07-t.svg platform/chromium/fast/text/text-stroke-with-border.html svg/W3C-SVG-1.1/animate-elem-08-t.svg svg/W3C-SVG-1.1/animate-elem-09-t.svg svg/W3C-SVG-1.1/animate-elem-14-t.svg platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html svg/W3C-SVG-1.1/animate-elem-11-t.svg svg/W3C-SVG-1.1/animate-elem-05-t.svg svg/W3C-SVG-1.1/animate-elem-02-t.svg svg/W3C-SVG-1.1/animate-elem-03-t.svg svg/W3C-SVG-1.1/animate-elem-06-t.svg svg/W3C-SVG-1.1/animate-elem-13-t.svg svg/W3C-SVG-1.1/animate-elem-10-t.svg 466987 13 husky 2011-09-14 06:27:20 -0700 The stylebot failure is because I'm indenting inside a namespace to match the surrounding code. Not fixable without updating the whole file. Running the layout tests locally, the SVG tests passed on one run but failed on the next. Looks like they're flaky. I don't think this patch affects them at all. 466993 14 husky 2011-09-14 06:37:37 -0700 Adding local committer Steve Block 467019 15 107153 steveblock 2011-09-14 07:19:50 -0700 Comment on attachment 107153 Patch Clearing flags on attachment: 107153 Committed r95090: <http://trac.webkit.org/changeset/95090> 467020 16 steveblock 2011-09-14 07:19:58 -0700 All reviewed patches have been landed. Closing bug. 106879 2011-09-09 09:23:33 -0700 2011-09-13 02:16:48 -0700 Patch bug-67851-20110909172332.patch text/plain 2299 husky SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTQ4NTYpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAK KzIwMTEtMDktMDkgIElhaW4gTWVycmljayAgPGh1c2t5QGdvb2dsZS5jb20+CisKKyAgICAgICAg V1RGU3RyaW5nLmggc2hvdWxkIGluY2x1ZGUgdGVtcGxhdGUgc3BlY2lhbGl6YXRpb24gZm9yIEhh c2hUcmFpdHM8U3RyaW5nPgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9Njc4NTEKKworICAgICAgICBUaGUgdGVtcGxhdGUgc3BlY2lhbGl6YXRpb24gd2Fz IGluIFN0cmluZ0hhc2guaCwgYnV0IHRoYXQgZmlsZSBpcyBub3QKKyAgICAgICAgYWx3YXlzIGlu Y2x1ZGVkIChpbiB0aGUgY29yZSBoZWFkZXJzLCBvbmx5IFdURlRocmVhZERhdGEuaCBpbmNsdWRl cyBpdCkuCisgICAgICAgIElmIFN0cmluZ0hhc2guaCB3ZXJlIG5vdCBpbmNsdWRlZCwgeW91IHdv dWxkIHBpY2sgdXAgdGhlIHdyb25nIEhhc2hUcmFpdHMKKyAgICAgICAgc3BlY2lhbGl6YXRpb24s IHdoaWNoIGlzIHByb2JhYmx5IGluZWZmaWNpZW50LiBJZiBTdHJpbmdIYXNoLmggd2VyZQorICAg ICAgICBpbmNsdWRlZCBhZnRlciBIYXNoU2V0PFN0cmluZz4gaGFzIGFscmVhZHkgYmVlbiB1c2Vk LCB5b3Ugd291bGQgZ2V0IGEKKyAgICAgICAgdGVtcGxhdGUgc3BlY2lhbGl6YXRpb24gY29tcGls ZSBlcnJvci4KKworICAgICAgICBUaGlzIHBhdGNoIG1vdmVzIHRoZSB0ZW1wbGF0ZSBzcGVjaWFs aXphdGlvbiBpbnRvIFdURlN0cmluZy5oIGl0c2VsZi4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHd0Zi90ZXh0L1N0cmluZ0hhc2guaDoKKyAgICAg ICAgKiB3dGYvdGV4dC9XVEZTdHJpbmcuaDoKKwogMjAxMS0wOS0wOCAgTWFyayBIYWhuZW5iZXJn ICA8bWhhaG5lbmJlcmdAYXBwbGUuY29tPgogCiAgICAgICAgIFJFR1JFU1NJT04ocjk0ODExKTog QXNzZXJ0aW9uIGZhaWx1cmUgaW4gMiB3b3JrZXIgdGVzdHMKSW5kZXg6IFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS93dGYvdGV4dC9TdHJpbmdIYXNoLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFT Y3JpcHRDb3JlL3d0Zi90ZXh0L1N0cmluZ0hhc2guaAkocmV2aXNpb24gOTQ4NTEpCisrKyBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvU3RyaW5nSGFzaC5oCSh3b3JraW5nIGNvcHkpCkBA IC0xNzksOCArMTc5LDYgQEAgbmFtZXNwYWNlIFdURiB7CiAgICAgICAgIH0KICAgICB9OwogCi0g ICAgdGVtcGxhdGU8PiBzdHJ1Y3QgSGFzaFRyYWl0czxTdHJpbmc+IDogU2ltcGxlQ2xhc3NIYXNo VHJhaXRzPFN0cmluZz4geyB9OwotCiB9CiAKIHVzaW5nIFdURjo6U3RyaW5nSGFzaDsKSW5kZXg6 IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvdGV4dC9XVEZTdHJpbmcuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvV1RGU3RyaW5nLmgJKHJldmlzaW9uIDk0 ODUxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi90ZXh0L1dURlN0cmluZy5oCSh3b3Jr aW5nIGNvcHkpCkBAIC0yNiw2ICsyNiw3IEBACiAvLyBvbiBzeXN0ZW1zIHdpdGhvdXQgY2FzZS1z ZW5zaXRpdmUgZmlsZSBzeXN0ZW1zLgogCiAjaW5jbHVkZSAiU3RyaW5nSW1wbC5oIgorI2luY2x1 ZGUgPHd0Zi9IYXNoVHJhaXRzLmg+CiAKICNpZmRlZiBfX09CSkNfXwogI2luY2x1ZGUgPG9iamMv b2JqYy5oPgpAQCAtNTAyLDYgKzUwMyw3IEBAIHRlbXBsYXRlPD4gc3RydWN0IERlZmF1bHRIYXNo PFN0cmluZz4gewogfTsKIAogdGVtcGxhdGUgPD4gc3RydWN0IFZlY3RvclRyYWl0czxTdHJpbmc+ IDogU2ltcGxlQ2xhc3NWZWN0b3JUcmFpdHMgeyB9OwordGVtcGxhdGU8PiBzdHJ1Y3QgSGFzaFRy YWl0czxTdHJpbmc+IDogU2ltcGxlQ2xhc3NIYXNoVHJhaXRzPFN0cmluZz4geyB9OwogCiAvLyBT aGFyZWQgZ2xvYmFsIGVtcHR5IHN0cmluZy4KIFdURl9FWFBPUlRfUFJJVkFURSBjb25zdCBTdHJp bmcmIGVtcHR5U3RyaW5nKCk7Cg== 107153 2011-09-13 02:16:56 -0700 2011-09-14 07:19:50 -0700 Patch bug-67851-20110913101655.patch text/plain 1967 husky SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTQ4NTYpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAK KzIwMTEtMDktMTMgIElhaW4gTWVycmljayAgPGh1c2t5QGdvb2dsZS5jb20+CisKKyAgICAgICAg SGFzaFRyYWl0cy5oIHNob3VsZCBpbmNsdWRlIHRlbXBsYXRlIHNwZWNpYWxpemF0aW9uIGZvciBX VEY6OlN0cmluZworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9Njc4NTEKKworICAgICAgICBFbnN1cmUgdGhhdCB0aGUgdGVtcGxhdGUgc3BlY2lhbGl6YXRp b24gZm9yIEhhc2hUcmFpdHM8U3RyaW5nPiBpcyBhbHdheXMKKyAgICAgICAgcGlja2VkIHVwLiAo UHJldmlvdXNseSBpdCB3YXMgcG9zc2libGUgdG8gaW5jbHVkZSBIYXNoU2V0IGFuZCBTdHJpbmcg YnV0CisgICAgICAgIG5vdCB0aGUgY29ycmVjdCBIYXNoVHJhaXRzLCBzbyB5b3Ugd291bGQgZ2V0 IGFuIGluZWZmaWNpZW50IHRlbXBsYXRlCisgICAgICAgIGluc3RhbnRpYXRpb24uKQorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL0hhc2hUcmFp dHMuaDoKKyAgICAgICAgKiB3dGYvdGV4dC9TdHJpbmdIYXNoLmg6CisKIDIwMTEtMDktMDggIE1h cmsgSGFobmVuYmVyZyAgPG1oYWhuZW5iZXJnQGFwcGxlLmNvbT4KIAogICAgICAgICBSRUdSRVNT SU9OKHI5NDgxMSk6IEFzc2VydGlvbiBmYWlsdXJlIGluIDIgd29ya2VyIHRlc3RzCkluZGV4OiBT b3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0hhc2hUcmFpdHMuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvd3RmL0hhc2hUcmFpdHMuaAkocmV2aXNpb24gOTQ4NTEpCisrKyBT b3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0hhc2hUcmFpdHMuaAkod29ya2luZyBjb3B5KQpAQCAt MjgsNiArMjgsOCBAQAogCiBuYW1lc3BhY2UgV1RGIHsKIAorICAgIGNsYXNzIFN0cmluZzsKKwog ICAgIHVzaW5nIHN0ZDo6cGFpcjsKICAgICB1c2luZyBzdGQ6Om1ha2VfcGFpcjsKIApAQCAtODks NiArOTEsNyBAQCBuYW1lc3BhY2UgV1RGIHsKICAgICB9OwogCiAgICAgdGVtcGxhdGU8dHlwZW5h bWUgUD4gc3RydWN0IEhhc2hUcmFpdHM8UmVmUHRyPFA+ID4gOiBTaW1wbGVDbGFzc0hhc2hUcmFp dHM8UmVmUHRyPFA+ID4geyB9OworICAgIHRlbXBsYXRlPD4gc3RydWN0IEhhc2hUcmFpdHM8U3Ry aW5nPiA6IFNpbXBsZUNsYXNzSGFzaFRyYWl0czxTdHJpbmc+IHsgfTsKIAogICAgIC8vIHNwZWNp YWwgdHJhaXRzIGZvciBwYWlycywgaGVscGZ1bCBmb3IgdGhlaXIgdXNlIGluIEhhc2hNYXAgaW1w bGVtZW50YXRpb24KIApJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi90ZXh0L1N0cmlu Z0hhc2guaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvU3Ry aW5nSGFzaC5oCShyZXZpc2lvbiA5NDg1MSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYv dGV4dC9TdHJpbmdIYXNoLmgJKHdvcmtpbmcgY29weSkKQEAgLTE3OSw4ICsxNzksNiBAQCBuYW1l c3BhY2UgV1RGIHsKICAgICAgICAgfQogICAgIH07CiAKLSAgICB0ZW1wbGF0ZTw+IHN0cnVjdCBI YXNoVHJhaXRzPFN0cmluZz4gOiBTaW1wbGVDbGFzc0hhc2hUcmFpdHM8U3RyaW5nPiB7IH07Ci0K IH0KIAogdXNpbmcgV1RGOjpTdHJpbmdIYXNoOwo=