Bug 67851 - WTFString.h should include template specialization for HashTraits<String>
Summary: WTFString.h should include template specialization for HashTraits<String>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-09 09:18 PDT by Iain Merrick
Modified: 2011-09-14 07:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.25 KB, patch)
2011-09-09 09:23 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2011-09-13 02:16 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Merrick 2011-09-09 09:18:34 PDT
WTFString.h should include template specialization for HashTraits<String>
Comment 1 Iain Merrick 2011-09-09 09:23:33 PDT
Created attachment 106879 [details]
Patch
Comment 2 Iain Merrick 2011-09-09 09:27:51 PDT
See also https://bugs.webkit.org/show_bug.cgi?id=67848 which has more detail on where I ran into this problem.
Comment 3 Sam Weinig 2011-09-10 17:21:00 PDT
I am not sure I understand why this is necessary.  Lots of file include <wtf/text/StringHash.h>.
Comment 4 Iain Merrick 2011-09-12 04:14:09 PDT
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.
Comment 5 Sam Weinig 2011-09-12 09:55:31 PDT
(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.)
Comment 6 Iain Merrick 2011-09-12 11:52:07 PDT
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.
Comment 7 Darin Adler 2011-09-12 16:01:15 PDT
(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.
Comment 8 Iain Merrick 2011-09-13 01:36:34 PDT
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.
Comment 9 Iain Merrick 2011-09-13 02:16:56 PDT
Created attachment 107153 [details]
Patch
Comment 10 Iain Merrick 2011-09-13 02:18:52 PDT
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.
Comment 11 WebKit Review Bot 2011-09-13 02:18:56 PDT
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 12 WebKit Review Bot 2011-09-13 03:12:23 PDT
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
Comment 13 Iain Merrick 2011-09-14 06:27:20 PDT
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.
Comment 14 Iain Merrick 2011-09-14 06:37:37 PDT
Adding local committer Steve Block
Comment 15 Steve Block 2011-09-14 07:19:50 PDT
Comment on attachment 107153 [details]
Patch

Clearing flags on attachment: 107153

Committed r95090: <http://trac.webkit.org/changeset/95090>
Comment 16 Steve Block 2011-09-14 07:19:58 PDT
All reviewed patches have been landed.  Closing bug.