[webkit-reviews] review granted: [Bug 229826] [JSC] Implement Temporal.Instant : [Attachment 442498] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 28 23:24:18 PDT 2021
Yusuke Suzuki <ysuzuki at apple.com> has granted Philip Chimento
<philip.chimento at gmail.com>'s request for review:
Bug 229826: [JSC] Implement Temporal.Instant
https://bugs.webkit.org/show_bug.cgi?id=229826
Attachment 442498: Patch
https://bugs.webkit.org/attachment.cgi?id=442498&action=review
--- Comment #34 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 442498
--> https://bugs.webkit.org/attachment.cgi?id=442498
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=442498&action=review
r=me with nits.
> Source/JavaScriptCore/runtime/TemporalInstant.cpp:109
> + if constexpr (sizeof(JSBigInt::Digit) == 4) {
> + Int128 d0 { bigint->length() > 0 ? bigint->digit(0) : 0 };
> + Int128 d1 { bigint->length() > 1 ? bigint->digit(1) : 0 };
> + Int128 d2 { bigint->length() > 2 ? bigint->digit(2) : 0 };
> + total = d2 << 64 | d1 << 32 | d0;
> + bigIntTooLong = bigint->length() > 3;
> + } else {
> + ASSERT(sizeof(JSBigInt::Digit) == 8);
> + Int128 low { bigint->length() > 0 ? bigint->digit(0) : 0 };
> + Int128 high { bigint->length() > 1 ? bigint->digit(1) : 0 };
> + total = high << 64 | low;
> + bigIntTooLong = bigint->length() > 2;
> + }
INT128_MIN case is not handled properly (-1 * INT128_MIN is undefined).
Can you check it? (and can you add a test?)
> Source/JavaScriptCore/runtime/TemporalInstant.cpp:233
> + int64_t microseconds = JSBigInt::toBigInt64(bigint);
It is not handling negative Int128 ranges (if JSBigInt's 2 digits are larger
than INT128_MAX, then if it is signed, it is not within Int128 range).
Can you fix it and add test for this?
> Source/JavaScriptCore/runtime/TemporalObject.cpp:470
> +
Let's add `ASSERT(increment != 0)`
More information about the webkit-reviews
mailing list