[webkit-reviews] review granted: [Bug 221245] [JSC] Atomics should support BigInt64Array / BigUint64Array : [Attachment 418962] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 221245: [JSC] Atomics should support BigInt64Array / BigUint64Array
https://bugs.webkit.org/show_bug.cgi?id=221245

Attachment 418962: Patch

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




--- Comment #3 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 418962
  --> https://bugs.webkit.org/attachment.cgi?id=418962
Patch

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

r=me but I have some comments.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:172
> +EncodedJSValue atomicReadModifyWrite(JSGlobalObject* globalObject, VM& vm,
const JSValue* args, const Func& func)

Why change the argument order here? We typically use VM as the first argument.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:338
> +    typename Adaptor::Type extraArgs[1];

Why is this an array? Just so you don't have to & in the StoreFunc? If so I
don't think that's worth it...

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:363
> +    JSArrayBufferView* typedArrayView =
validateIntegerTypedArray<TypedArrayOperationMode::Read>(globalObject, base);

Can we change the enum values? The current ones don't make any sense... why is
atomicStore checking Read??

I'd name them ReadWrite and Wait(able), but I'm flexible.