Bug 147866 - Implement IntegerHasher
Summary: Implement IntegerHasher
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
Depends on:
Reported: 2015-08-10 21:23 PDT by Myles C. Maxfield
Modified: 2018-11-06 03:43 PST (History)
6 users (show)

See Also:

Patch (1.86 KB, patch)
2015-08-10 21:26 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff
Patch (39.79 KB, patch)
2015-08-17 15:55 PDT, Myles C. Maxfield
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-08-10 21:23:25 PDT
[font-features] Addressing post-commit review of r188088
Comment 1 Myles C. Maxfield 2015-08-10 21:26:19 PDT
Created attachment 258698 [details]
Comment 2 Darin Adler 2015-08-10 22:49:54 PDT
Comment on attachment 258698 [details]

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

Side questions:

1: Why isn’t m_tag in FontFeature marked const?

2: Generally speaking, FontFeature seems to be a custom-written pair; is there a way to use more of std::pair in its implementation?

3: Why so many includes in this header? I don’t think we need to include anything other than Vector.h and AtomicString.h.

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:79
> +    static_assert(sizeof(unsigned) == sizeof(UChar) * 2, "Must be able to use StringHasher::addCharacters()");
> +    for (size_t i = 0; i < size(); ++i) {
> +        unsigned featureHash = at(i).hash();
> +        UChar lower = featureHash;
> +        UChar upper = featureHash >> (sizeof(UChar) * 8);
> +        hasher.addCharacters(upper, lower);
> +    }

I’d write this:

    for (auto& feature : m_list) {
        unsigned hash = feature.hash();
        hasher.addCharactersAssumingAligned(hash, hash >> 16);

I don’t think we should try to be abstract about how many bits there are in a UChar. And there's no reason to compile in the code to handle unaligned hashing. If you want to make it even cleaner, we could write something called IntegerHasher, like this:

    class IntegerHasher {
        void add(unsigned integer) { m_underlyingHasher.addCharactersAssumingAligned(integer, integer >> 16); }
        unsigned hash() const { return m_underlyingHasher.hash(); }
        StringHasher m_underlyingHasher;


    for (auto& feature : m_list)

Finally, I’m not really sure it’s good to be hashing hashes so excessively. Maybe use the hashes of the tag strings, but for the rest it seems excessive to hash just so we can hash again. Maybe like this:

    for (auto& feature : m_list) {
Comment 3 Myles C. Maxfield 2015-08-17 15:55:21 PDT
Created attachment 259196 [details]
Comment 4 Anders Carlsson 2015-08-17 15:57:26 PDT
Comment on attachment 259196 [details]

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

> Source/WTF/wtf/Hasher.h:295
> +

No need for a newline here.
Comment 5 Myles C. Maxfield 2015-08-17 16:59:13 PDT
Committed r188557: <http://trac.webkit.org/changeset/188557>
Comment 6 Frédéric Wang (:fredw) 2018-11-06 03:43:02 PST
Comment on attachment 259196 [details]

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

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:67
> +    IntegerHasher hasher;

This bug is very old but probably we needed to include <wtf/Hasher.h> in order to use IntegerHash here. I just landed a commit adding this header.