[webkit-reviews] review granted: [Bug 237049] Drop StringHasher::hashMemory() and use the modern Hasher instead : [Attachment 452895] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 237049: Drop StringHasher::hashMemory() and use the modern Hasher instead
https://bugs.webkit.org/show_bug.cgi?id=237049

Attachment 452895: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 452895
  --> https://bugs.webkit.org/attachment.cgi?id=452895
Patch

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

> Source/WTF/wtf/SchedulePair.h:77
> +	   return computeHash(reinterpret_cast<uintptr_t>(pair->runLoop()),
pair->mode() ? CFHash(pair->mode()) : 0);

Not new, but we should probably not hash a CFHash() here and should just hash
the CFStringRef (not needed for this change, just pointing it out).

> Source/WTF/wtf/UUID.h:101
> +    add(hasher, high, low);

Probably worth just adding an overload of add(Hasher, ..) for UInt128. But I
think you can also use the UInt128Low64/UInt128High64 helpers instead of the
explicit bit operations.

> Source/WebCore/Modules/indexeddb/shared/IDBResourceIdentifier.h:93
> -    static unsigned hash(const IDBResourceIdentifier& a) { return a.hash();
}
> +    static unsigned hash(const IDBResourceIdentifier& a) { return
computeHash(a); }

Would be nice if we could avoid having this and instead have HashTable (or
whatever is using these Hash structs) try to call add(Hasher,...) if it exists.

> Source/WebCore/page/SecurityOriginHash.h:41
> +	   return computeHash(origin->protocol(), origin->host(),
origin->port().value_or(0));

This will compute a different hash value than the old one, and will be more
expensive due to no using cached hash values in the strings. This is likely a
better hash overall since you get more entropy per-byte, but it might be a slow
down due to rehashing each string.

> Source/WebKitLegacy/mac/History/BinaryPropertyList.cpp:81
> +    auto* integers = array.integers();
> +    for (size_t i = 0; i < array.size(); ++i)
> +	   add(hasher, integers[i]);

I would be nice if this could be done instead:

    add(hasher, Span(array.integers(), array.size()));