| Summary: | [JSC] >>> should call ToNumeric | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2020-04-27 02:00:40 PDT
Created attachment 397662 [details]
Patch
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 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? 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. 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. 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. Committed r260805: <https://trac.webkit.org/changeset/260805> |