Summary: | WTFString.h should include template specialization for HashTraits<String> | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Iain Merrick <husky> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, dglazkov, husky, jamesr, kbr, mjs, sam, steveblock, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Iain Merrick
2011-09-09 09:18:34 PDT
Created attachment 106879 [details]
Patch
See also https://bugs.webkit.org/show_bug.cgi?id=67848 which has more detail on where I ran into this problem. I am not sure I understand why this is necessary. Lots of file include <wtf/text/StringHash.h>. 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. (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.) 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. (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. 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. Created attachment 107153 [details]
Patch
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. Attachment 107153 [details] 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.
Comment on attachment 107153 [details] Patch Attachment 107153 [details] 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 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. Adding local committer Steve Block Comment on attachment 107153 [details] Patch Clearing flags on attachment: 107153 Committed r95090: <http://trac.webkit.org/changeset/95090> All reviewed patches have been landed. Closing bug. |