[webkit-reviews] review granted: [Bug 110171] StringHasher functions require alignment that call sites do not all guarantee : [Attachment 188974] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Darin Adler
<darin at apple.com>'s request for review:
Bug 110171: StringHasher functions require alignment that call sites do not all
guarantee
https://bugs.webkit.org/show_bug.cgi?id=110171

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188974&action=review


The new methods addCharactersAssumingAligned() are public. Wouldn't it be
better to have them private until they are needed to be public by client code?
Exposing those low level functions increases the risk of misuses like the one
you fix with this patch.

If you keep the new methods public, it would be nice to have new test coverage
for them.

> Source/WTF/wtf/StringHasher.h:77
> -	       addCharactersToHash(m_pendingCharacter, Converter(*data++));
> -	       m_hasPendingCharacter = false;
> -	       --length;
> +	       addCharactersInternal(m_pendingCharacter, a);
> +	       m_pendingCharacter = b;
> +	       return;

It would be nice to get rid of addCharactersInternal to simplify the class.
Is there really any extra cost to?:
    m_hasPendingCharacter = false;
    addCharactersInternal(m_pendingCharacter, a);
    m_pendingCharacter = b;
    m_hasPendingCharacter = true;