Bug 211065 – [JSC] >>> should call ToNumeric

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

[Yusuke Suzuki]

While BigInt does not support >>>, we need to call ToNumeric instead of ToNumber.

[Yusuke Suzuki]

Created attachment 397662 [details]
Patch

[Ross Kirsling]

Comment on attachment 397662 [details]
Patch

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

r=me

It would be good to have a spec link in the ChangeLog.
Presumably: https://tc39.es/ecma262/#sec-unsigned-right-shift-operator

> Source/JavaScriptCore/ChangeLog:10
> +        since the only difference between toUint32 and toInt32 is casting int32_t result to uint32_t.

This is interesting.

It's funny that this comment claims that there will be an explanation:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSCJSValueInlines.h#L56
But it must have been removed?
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSCJSValue.h#L293

[Darin Adler]

Comment on attachment 397662 [details]
Patch

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

> Source/JavaScriptCore/runtime/Operations.h:842
> +    uint32_t leftUint32 = static_cast<uint32_t>(leftNumeric.asInt32());
> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());

Why asInt32 and casting to uint32_t rather than asUInt32? If asUInt32 is not for this, then why do we have it?

[Yusuke Suzuki]

Comment on attachment 397662 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/Operations.h:842
>> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());
> 
> Why asInt32 and casting to uint32_t rather than asUInt32? If asUInt32 is not for this, then why do we have it?

asUInt32() is used for different purpose, mainly for array index, and it has ASSERT that int32 value is positive. This is different from toUInt32 semantics since it reinterpret negative int32 bit pattern as uint32.

[Darin Adler]

Comment on attachment 397662 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/Operations.h:842
>>> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());
>> 
>> Why asInt32 and casting to uint32_t rather than asUInt32? If asUInt32 is not for this, then why do we have it?
> 
> asUInt32() is used for different purpose, mainly for array index, and it has ASSERT that int32 value is positive. This is different from toUInt32 semantics since it reinterpret negative int32 bit pattern as uint32.

Is there some way to not have the typecast be here? I think that the point of having functions like JSC::toUInt32 and JSValue::asUInt32 is to keep the typecasts in a smaller number of places. I understand that none of them are quite right for a case where we know isInt32 is true and want to do the toUInt32 operation.

So annoying that JSValue::toUInt32 has a comment in it saying we should look for another comment, but it mentions the wrong header: JSCValue.h rather than MathCommon.h.

[Yusuke Suzuki]

Comment on attachment 397662 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        since the only difference between toUint32 and toInt32 is casting int32_t result to uint32_t.
> 
> This is interesting.
> 
> It's funny that this comment claims that there will be an explanation:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSCJSValueInlines.h#L56
> But it must have been removed?
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSCJSValue.h#L293

Seems super old comment, and maybe removed at some point.
Added comment with https://tc39.es/ecma262/#sec-touint32.

>>>> Source/JavaScriptCore/runtime/Operations.h:842
>>>> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());
>>> 
>>> Why asInt32 and casting to uint32_t rather than asUInt32? If asUInt32 is not for this, then why do we have it?
>> 
>> asUInt32() is used for different purpose, mainly for array index, and it has ASSERT that int32 value is positive. This is different from toUInt32 semantics since it reinterpret negative int32 bit pattern as uint32.
> 
> Is there some way to not have the typecast be here? I think that the point of having functions like JSC::toUInt32 and JSValue::asUInt32 is to keep the typecasts in a smaller number of places. I understand that none of them are quite right for a case where we know isInt32 is true and want to do the toUInt32 operation.
> 
> So annoying that JSValue::toUInt32 has a comment in it saying we should look for another comment, but it mentions the wrong header: JSCValue.h rather than MathCommon.h.

I've added `Optional<uint32_t> JSValue::toUInt32AfterToNumeric(JSGlobalObject*) const;`, which calls toBigIntOrInt32 internally, and returns uint32_t if the result is Int32. Otherwise, return WTF::nullopt.
Looks like the comment in toUInt32 is super old (more than 7 years ago), and some refactoring made that comment stale. Yeah, that's annoying.
I've just add comment which just describes that only difference in toUInt32 / toInt32 is that toUInt32 just reinterprets int32_t result as uint32_t.

[Yusuke Suzuki]

Committed r260805: <https://trac.webkit.org/changeset/260805>

[Radar WebKit Bug Importer]

<rdar://problem/62478977>