Bug 67851 – WTFString.h should include template specialization for HashTraits<String>

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

WTFString.h should include template specialization for HashTraits<String>

[Iain Merrick]

Created attachment 106879 [details]
Patch

[Iain Merrick]

See also https://bugs.webkit.org/show_bug.cgi?id=67848 which has more detail on where I ran into this problem.

[Sam Weinig]

I am not sure I understand why this is necessary.  Lots of file include <wtf/text/StringHash.h>.

[Iain Merrick]

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.

[Sam Weinig]

(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.)

[Iain Merrick]

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.

[Darin Adler]

(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.

[Iain Merrick]

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.

[Iain Merrick]

Created attachment 107153 [details]
Patch

[Iain Merrick]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Iain Merrick]

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.

[Iain Merrick]

Adding local committer Steve Block

[Steve Block]

Comment on attachment 107153 [details]
Patch

Clearing flags on attachment: 107153

Committed r95090: <http://trac.webkit.org/changeset/95090>

[Steve Block]

All reviewed patches have been landed.  Closing bug.