Bug 154352 – Extend HashCountedSet with a method to efficiently set the count of an entry

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

When working on Bug 153575, I had to add some silly code like the following:

        // FIXME: Create a HashCountedSet method to do this efficiently
        for (unsigned i = 0; i < count; ++i)
            hashCountedSet.add(origin);

This bug adds a method to support seeding the HashCountedSet with the appropriate count.

[Brent Fulgham]

Created attachment 271590 [details]
Patch

[Geoffrey Garen]

Comment on attachment 271590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271590&action=review

> Source/WTF/wtf/HashCountedSet.h:166
> +    inline typename HashCountedSet<Value, HashFunctions, Traits>::AddResult HashCountedSet<Value, HashFunctions, Traits>::addWithInitialCount(const ValueType& value, unsigned initialCount)
> +    {
> +        return m_impl.add(value, initialCount);
> +    }

This is slightly wrong -- and also a bit surprising.

If the set already contains the object, this will do nothing.

You should either call this function set, and have it call through to set, or you should call it add, and have it add 0, and then increment the resulting iterator by count, similar to the code above that adds 1.

[Brent Fulgham]

Comment on attachment 271590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271590&action=review

It's also missing tests. I'm fixing both issues.

>> Source/WTF/wtf/HashCountedSet.h:166
>> +    }
> 
> This is slightly wrong -- and also a bit surprising.
> 
> If the set already contains the object, this will do nothing.
> 
> You should either call this function set, and have it call through to set, or you should call it add, and have it add 0, and then increment the resulting iterator by count, similar to the code above that adds 1.

OK!

[Brent Fulgham]

While building a test suite for this class, I noticed that many of the data types accepted by HashMap are improperly treated by this wrapper class. I'll update this class to provide move operators and an initializer list constructor to support the same features as its core implementation class.

[Brent Fulgham]

Created attachment 271667 [details]
Patch

[WebKit Commit Bot]

Attachment 271667 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #6)
> Attachment 271667 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> Total errors found: 5 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

These template declarations match the style of the rest of the file. I'm not sure if we want to use a different style for just those lines.

[Brent Fulgham]

It looks like Visual Studio doesn't like the 'template' syntax. I've got a build fix on Windows, which I need to re-test on Mac.

[Brent Fulgham]

Created attachment 271714 [details]
Patch

[Brent Fulgham]

Created attachment 271715 [details]
Patch (With Windows fixes)

[WebKit Commit Bot]

Attachment 271715 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 271715 [details]
Patch (With Windows fixes)

r=me

[Brent Fulgham]

Committed r196791: <http://trac.webkit.org/changeset/196791>

[Csaba Osztrogonác]

(In reply to comment #13)
> Committed r196791: <http://trac.webkit.org/changeset/196791>

It broke tge Windows build.

[Alex Christensen]

Comment on attachment 271715 [details]
Patch (With Windows fixes)

View in context: https://bugs.webkit.org/attachment.cgi?id=271715&action=review

> Source/WTF/wtf/HashCountedSet.h:290
> +        return m_impl.template find(value);

Visual Studio doesn't like this syntax.

> Source/WTF/wtf/HashCountedSet.h:321
> -} // namespace khtml
> +} // namespace WTF

The last namespace khtml comment is gone :(

[WebKit Commit Bot]

Re-opened since this is blocked by bug 154438

[Brent Fulgham]

(In reply to comment #15)
> Comment on attachment 271715 [details]
> Patch (With Windows fixes)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271715&action=review
> 
> > Source/WTF/wtf/HashCountedSet.h:290
> > +        return m_impl.template find(value);
> 
> Visual Studio doesn't like this syntax.

Ugh! I had this fix on my Windows machine, but I landed from my Mac. >:-(

[Brent Fulgham]

Committed r196802: <http://trac.webkit.org/changeset/196802>

[Brent Fulgham]

Note: This might need a clean build. It looks like the "template" error was showing after the rollout, so it's possible that my recent fix will also result in at least one "stale" build failure.

[Joseph Pecoraro]

Comment on attachment 271715 [details]
Patch (With Windows fixes)

View in context: https://bugs.webkit.org/attachment.cgi?id=271715&action=review

>> Source/WTF/wtf/HashCountedSet.h:321
>> +} // namespace WTF
> 
> The last namespace khtml comment is gone :(

!