[webkit-reviews] review granted: [Bug 91891] QualifiedName's HashSet should be big enough to hold at least all the static names : [Attachment 153870] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 91891: QualifiedName's HashSet should be big enough to hold at least all
the static names
https://bugs.webkit.org/show_bug.cgi?id=91891

Attachment 153870: Patch
https://bugs.webkit.org/attachment.cgi?id=153870&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=153870&action=review


r=me, but I think there is some room for improvement

> Source/WTF/wtf/HashTable.h:478
> +    // The following computes the upper power of two capacity to hold the
size parameter.
> +    // This is done at compile time to initialize the HashTraits.
> +    template<unsigned size> struct
hashTableCapacityForSizeUpperPowerOfTwoMinusOne;
> +    template<>
> +    struct hashTableCapacityForSizeUpperPowerOfTwoMinusOne<0> {
> +	   static const unsigned value = 0;
> +    };
> +    template<unsigned size>
> +    struct hashTableCapacityForSizeUpperPowerOfTwoMinusOne {
> +	   static const unsigned value = size |
hashTableCapacityForSizeUpperPowerOfTwoMinusOne<(size >> 1)>::value;
> +    };
> +
> +    template<unsigned size, bool isPowerOfTwo> struct
hashTableCapacityForSizeDecider;
> +    template<unsigned size>
> +    struct hashTableCapacityForSizeDecider<size, true> {
> +	   static const unsigned value = size << 2;
> +    };
> +    template<unsigned size>
> +    struct hashTableCapacityForSizeDecider<size, false> {
> +	   static const unsigned value =
(hashTableCapacityForSizeUpperPowerOfTwoMinusOne<size - 1>::value + 1) << 1;
> +    };

There must be a less confusing way to code this.

I have no idea what “upper power of two minus one” means, nor do I why powers
of two need a special case. I think the code is doing some kind of smearing
technique to construct a binary integer of all ones, but this is so non-obvious
that it requires a comment.

I suggest not repeating “ForSize” in all these struct names, and there seems no
reason for hashTableCapacityForSizeUpperPowerOfTwoMinusOne to have
hashTableCapacity in its name. It’s just an arithmetic function.

> Source/WTF/wtf/HashTable.h:481
> +    struct hashTableCapacityForSize {

Name should be capitalized. All the other traits structs and classes are. Even
though they are “function-like”.

> Source/WTF/wtf/HashTable.h:485
> +	   COMPILE_ASSERT(value > (2 * size),
HashTableCapacityHoldsContentSize);

Shouldn’t this be >=?

> Source/WebCore/dom/QualifiedName.cpp:35
> +#if ENABLE(MATHML)
> +#include "MathMLNames.h"
> +#endif
> +#if ENABLE(SVG)
> +#include "SVGNames.h"
> +#endif

Normally we put conditional includes in their own separate paragraphs.

> Source/WebCore/dom/QualifiedName.cpp:47
> +    static_cast<const unsigned>(HTMLNames::HTMLTagsCount +
HTMLNames::HTMLAttrsCount

I don’t think the const here is needed.

> Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp:54
> +    // Adding one more item may change the capacity.
> +    testSet.add(initialCapacity);
> +    EXPECT_GE(static_cast<unsigned>(testSet.capacity()), initialCapacity);

I think this would be a better test if it added more than just one more item.
In fact, might be good to keep adding until the capacity goes up and make some
assertion about when that happens.