[webkit-reviews] review granted: [Bug 228962] Update ARM64EHash : [Attachment 435377] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 11 22:29:24 PDT 2021

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 228962: Update ARM64EHash

Attachment 435377: patch


--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 435377
  --> https://bugs.webkit.org/attachment.cgi?id=435377

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

r=me with some comments.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:213
> +	       return static_cast<PtrTag>((static_cast<uint64_t>(namespaceTag)
<< 56) + ((index & 0xFFFFFF) << 32) + static_cast<uint64_t>(value));

I think the static_cast in `static_cast<uint64_t>(value)` is not needed.  It
will automatically be promoted.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:251
> +	   ARM64EHash(void* diversifier)
> +	   {
> +	       setUpdatedHash(0, 0, diversifier);
> +	   }

nit: can you put this at the top of the class?	Seems weird to have the
constructor hidden in the middle of all the other methods.

Also, all the methods above this constructor can be moved into the private
section below.	I think that makes it clearer that they are not part of the
public interface.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:432
>	       WTF::unalignedStore<uint32_t>(m_hashes.buffer() + m_index,

Funny: this only works because IntegralType == uint32_t because m_index is
incremented in sizeof(IntegralType), and this buffer is incremented in
sizeof(uint32_t).  Anyway, it's not due to your patch, and in practice,
IntegralType is always uint32_t here.

More information about the webkit-reviews mailing list