Bug 154352

Created attachment 271590 [details] Patch

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.

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!

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.

Created attachment 271667 [details] Patch

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.

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

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.

Created attachment 271714 [details] Patch

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

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.

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

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

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

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

Re-opened since this is blocked by bug 154438

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

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

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.

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 :( !