[webkit-reviews] review granted: [Bug 210755] [JSC] BigInt constructor should accept larger integers than safe-integers : [Attachment 397916] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 210755: [JSC] BigInt constructor should accept larger integers than
safe-integers
https://bugs.webkit.org/show_bug.cgi?id=210755

Attachment 397916: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 397916
  --> https://bugs.webkit.org/attachment.cgi?id=397916
Patch

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

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:130
> +	   if (std::abs(number) <= maxSafeInteger())
> +	       return JSValue::encode(JSBigInt::makeHeapBigIntOrBigInt32(vm,
static_cast<int64_t>(primitive.asDouble())));
> +	   return JSValue::encode(JSBigInt::makeHeapBigIntOrBigInt32(vm,
primitive.asDouble()));

Unclear why the maxSafeInteger special case here is valuable, with a conversion
to int64_t, but that same special case is not a good optimization inside the
makeHeapBigIntOrBigInt32 overload that takes a double. I suggest leaving the
maxSafeInteger check out here entirely, and either include it, or do not,
inside the makeHeapBigIntOrBigInt32 function.

Unless I am missing a reason why it’s more helpful here.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:221
> +    const int32_t mantissaTopBit = doubleMantissaSize - 1; // 0-indexed.
> +    // 0-indexed position of most significant bit in the most significant
digit.
> +    int32_t msdTopBit = exponent % digitBits;

Unclear to me why we use const in one place and not the other. I would just
omit it in both places, but no big deal.

> Source/JavaScriptCore/runtime/JSBigInt.h:112
> +	   auto ptr = JSBigInt::createFrom(vm, value);
> +	   return JSValue(static_cast<JSCell*>(ptr));

Why is this two lines? Why is the static_cast needed? Why is the explicit
conversion to JSValue needed?

> Source/JavaScriptCore/runtime/MathCommon.h:51
> +inline bool isInteger(double value)

Can this be constexpr? I guess not, because std::isfinite and std::trunc are
not available in constexpr versions.

Slightly dangerous to have a function named isInteger that takes a double. You
could call it on a float and get poor performance. Overloading for float would
make that danger go away. Or using a template. Or something else that would
turn off conversions. I wish there was a way to say "really gotta be a double"
for the argument and disable conversions.

> Source/JavaScriptCore/runtime/MathCommon.h:53
> +    return std::isfinite(value) && trunc(value) == value;

Strange to mix std::isfinite with trunc. I think we should use std:: on both or
neither.

> Source/JavaScriptCore/runtime/MathCommon.h:58
> +    return isInteger(value) && std::abs(value) <= maxSafeInteger();

Seems unnecessary to assume that std::abs(minSafeInteger()) ==
maxSafeInteger(). We could static_assert it, if this is really an important
optimization, or compare against both min and max.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:150
>      return JSValue::encode(jsBoolean(isInteger));

Local variable should not be named isInteger. That’s actively incorrect, since
it will be false for integers that are not safe! It should be named
isSafeInteger, or result, or something that is not inaccurate.

> Source/JavaScriptCore/runtime/NumberConstructor.h:56
>	       return true;
>	   if (!value.isDouble())
>	       return false;
> -
> -	   double number = value.asDouble();
> -	   return std::isfinite(number) && trunc(number) == number;
> +	   return isInteger(value.asDouble());

I like boolean expressions:

    return value.isInt32() || (value.isDouble() &&
isInteger(value.asDouble()));

Look how pretty that is!

> JSTests/ChangeLog:25
> +2020-04-28  Yusuke Suzuki  <ysuzuki at apple.com>
> +
> +	   [JSC] BigInt constructor should accept larger integers than
safe-integers
> +	   https://bugs.webkit.org/show_bug.cgi?id=210755
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * stress/large-number-to-bigint.js: Added.
> +	   (shouldBe):
> +	   (shouldThrow):
> +	   * test262/config.yaml:
> +

Double change log.

> JSTests/stress/large-number-to-bigint.js:42
> +shouldBe(BigInt(Number.MAX_SAFE_INTEGER), 9007199254740991n);
> +shouldBe(BigInt(Number.MIN_SAFE_INTEGER), -9007199254740991n);

How about testing the ones that are just past the safe integers?