[webkit-reviews] review granted: [Bug 180318] Add WTF::Hasher, an easier to use replacement for WTF::IntegerHasher : [Attachment 328254] Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 180318: Add WTF::Hasher, an easier to use replacement for
WTF::IntegerHasher
https://bugs.webkit.org/show_bug.cgi?id=180318

Attachment 328254: Patch

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




--- Comment #5 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 328254
  --> https://bugs.webkit.org/attachment.cgi?id=328254
Patch

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

r=me with a few comments, and a design suggestion.

As we discussed in person I think it would be cool to rely on the tuple-like
type API (with either member or non-member `get` functions), because once we
move to C++17 all functions that implemented that API get structured binding
for free (case #2 in [1]).

Looking at your code though I'm not sure how much uglier it would then look,
since what you have now is pretty elegant. It can be added later, but once too
many classes don't do something it gets hard to migrate. Your call :-)

  [1]: http://en.cppreference.com/w/cpp/language/structured_binding

> Source/WTF/wtf/Hasher.h:31
> +    void add(unsigned integer)

It's a bit odd to mix unsigned here and for hash, with uint32_t below. I'd go
for uint32_t everywhere.

> Source/WTF/wtf/Hasher.h:65
> +    template<typename UnsignedInteger> friend void
add(std::enable_if_t<std::is_unsigned<UnsignedInteger>::value &&
sizeof(UnsignedInteger) <= sizeof(uint32_t), Hasher&> hasher, UnsignedInteger
integer)

Just personal preference, but I prefer using enable_if for the return type
instead of on parameters because I find it easier to read (that is, unless it
can be a default template parameter which you can't do here because you're
overloading `add`).

> Source/WTF/wtf/Hasher.h:68
> +	   // We can consider adding a more efficient code path for hashing
16-bit values if needed, perhaps using addCharacter,

FIXME follow-up bugs for those?

> Source/WTF/wtf/Hasher.h:90
> +inline void add(Hasher& hasher, double number)

One question about hashing floating-point values is what happens if you hash
-0. / +0. (both compare equal) and NaN values (which never compare equal). Do
you think we ever do this, and would it be a bug to do so? Maybe you want to
normalize zeros to positive, or keep the distinction so they do hash
differently? For NaNs is seems like you'd want to canonicalize them to what JSC
calls "pure" NaN?

> Source/WTF/wtf/Hasher.h:92
> +    add(hasher, reinterpret_cast<uint64_t&>(number));

You want bitwise_cast here and below.

> Source/WTF/wtf/Hasher.h:111
> +    for (auto& value : container)

const auto& (I know it already is, I just like to say it explicitly).

> Tools/TestWebKitAPI/Tests/WTF/Hasher.cpp:49
> +}

?

> Tools/TestWebKitAPI/Tests/WTF/Hasher.cpp:55
> +    EXPECT_EQ(zero32BitHash, computeHash(static_cast<char>(0)));

Here and below, you should also test signed char and unsigned char since
they're distinct from char.

> Tools/TestWebKitAPI/Tests/WTF/Hasher.cpp:129
> +    EXPECT_EQ(zero64BitHash, computeHash(0.0));

As discussed above, I'd figure out what we want from -0. and if it's the same
as +0. (won't be if you just bitwise_cast).

> Tools/TestWebKitAPI/Tests/WTF/Hasher.cpp:141
> +    EXPECT_EQ(2678086759U,
computeHash(std::numeric_limits<double>::quiet_NaN()));

I'd test other NaNs as well.