Bug 221245 – [JSC] Atomics should support BigInt64Array / BigUint64Array

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 418962 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 418962 [details]
Patch

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

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:354
> +    RELEASE_AND_RETURN(scope, JSValue::encode(value));

We need to return this |value|, not the value after `toNativeFromValue`'s enforcement is done.

[Keith Miller]

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

[Mark Lam]

Comment on attachment 418962 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        2. Extend Atomics.isLockFree to accept 8.

I suggest adding a bit more detail: "to also accept a size of 8".

[Yusuke Suzuki]

Comment on attachment 418962 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:12
>> +        2. Extend Atomics.isLockFree to accept 8.
> 
> I suggest adding a bit more detail: "to also accept a size of 8".

Added

>> 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.

We put JSGlobalObject* first when the function can throw an error.

>> 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...

This is for StoreFunc's signature. But since StoreFunc is only sued here, we can remove StoreFunc and just use atomicStoreFullyFenced directly here. Changed

>> 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.

Oops, yeah. Fixed to ReadWrite / Wait.

[Yusuke Suzuki]

Committed r272341: <https://trac.webkit.org/changeset/272341>

[Radar WebKit Bug Importer]

<rdar://problem/73951609>