[webkit-reviews] review granted: [Bug 196681] HashTable::removeIf always shrinks the hash table by half even if there is nothing left : [Attachment 367348] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 196681: HashTable::removeIf always shrinks the hash table by half even if
there is nothing left
https://bugs.webkit.org/show_bug.cgi?id=196681

Attachment 367348: Fixes the bug

https://bugs.webkit.org/attachment.cgi?id=367348&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 367348
  --> https://bugs.webkit.org/attachment.cgi?id=367348
Fixes the bug

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

> Source/WTF/wtf/HashTable.h:467
> +	   unsigned computeBestTableSize(unsigned keyCount) { return
WTF::roundUpToPowerOfTwo(keyCount) * 2; }

I think this should be should be static (and constexpr) member function, rather
than a non-static.

Is it safe to call roundUpToPowerOfTwo on the key count? Is there a guarantee
it won’t overflow?

> Source/WTF/wtf/HashTable.h:477
> +	   void shrinkToBestSize()
> +	   {
> +	       unsigned minimumTableSize = KeyTraits::minimumTableSize;
> +	       rehash(std::max<unsigned>(minimumTableSize,
computeBestTableSize(m_keyCount + 1)), nullptr);
> +	   }

Why does this add one to m_keyCount? The copy constructor doesn’t do that.

Unclear why it’s correct to do this computation without the
"aboveThreeQuarterLoad" check that is in the copy constructor.

Is it safe to add one to m_keyCount? Is there something that guarantees it
won’t overflow?

The <unsigned> is not needed in the call to std::max since both arguments are
unsigned. Same in the code this was copied from, in the copy constructor.

Unclear why we need minimumTableSize to be put into a local variable. Same in
the code this was copied from, in the copy constructor.

Style thought: I think a multi-line function should be defined outside the
class, so it doesn’t take up four lines in the class definition.

> Tools/ChangeLog:12
> +	(WTF_HashSet.RemoveIfShrinkToBestSize):

Looks like there’s a tab here.