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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 228962: Update ARM64EHash
https://bugs.webkit.org/show_bug.cgi?id=228962

Attachment 435377: patch

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




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

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,
hash);

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.