Bug 228962 – Update ARM64EHash

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://79883337>

[Saam Barati]

Created attachment 435377 [details]
patch

[Mark Lam]

Comment on attachment 435377 [details]
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.

[Mark Lam]

Comment on attachment 435377 [details]
patch

I think this patch is good on its own.  Since Saam is away, I'll just land this as is and implement my nits as a follow up refactoring patch later.

[EWS]

Committed r280984 (240484@main): <https://commits.webkit.org/240484@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435377 [details].

[Darin Adler]

Comment on attachment 435377 [details]
patch

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

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:220
> +            return static_cast<uint32_t>((a >> 39) ^ (b >> 23));

I’m surprised the static_cast to uint32_t here needed and valuable. Does it silence a truncation warning?

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:225
> +            return static_cast<uint32_t>(bitwise_cast<uintptr_t>(diversifier));

Ditto.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:236
> +            return static_cast<uint32_t>(result);

Ditto.

[Mark Lam]

Comment on attachment 435377 [details]
patch

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

>> Source/JavaScriptCore/assembler/AssemblerBuffer.h:220
>> +            return static_cast<uint32_t>((a >> 39) ^ (b >> 23));
> 
> I’m surprised the static_cast to uint32_t here needed and valuable. Does it silence a truncation warning?

These static_casts appear to not be needed in my local build test i.e. Clang does not complain.  I'll remove them in my upcoming refactoring patch.

[Mark Lam]

Will do refactoring change in https://bugs.webkit.org/show_bug.cgi?id=229054.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 229384

[Saam Barati]

Created attachment 436649 [details]
patch

[Mark Lam]

Comment on attachment 436649 [details]
patch

r=me

[EWS]

Committed r281717 (241059@main): <https://commits.webkit.org/241059@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436649 [details].