[webkit-reviews] review granted: [Bug 181144] [JSC] Implement BigInt.asIntN and BigInt.asUintN : [Attachment 398399] 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 181144: [JSC] Implement BigInt.asIntN and BigInt.asUintN
https://bugs.webkit.org/show_bug.cgi?id=181144

Attachment 398399: Patch

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




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

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

I didn’t carefully read the algorithms. I’m counting on the test cases instead.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:105
> +    throwTypeError(globalObject, scope, "Invalid primitive type in ToBigInt
operation"_s);

Is this an improvement in the error message over "Invalid argument type"? Or is
it just a little too much global replacing?

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:141
> +    uint32_t bits = callFrame->argument(0).toIndex(globalObject, "bits");

This is the kind of place where I love "auto". Because Otherwise I ask myself,
"is uint32_t the right type for the result of toIndex?"

I know the standard calls this argument "bits" but I would call it "number of
bits" or "bit count". Because this is not actually "bits".

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:145
> +    JSValue bigInt = toBigInt(globalObject, callFrame->argument(1));
> +    RETURN_IF_EXCEPTION(scope, { });

Do we want a fast case that avoids ever creating the BigInt? Seems like a long
round trip for, say, a boolean or an integer. I guess, on reflection, BigInt32
is that optimization. Still seems a shame to me, though.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:396
> +ALWAYS_INLINE JSBigInt::ImplResult JSBigInt::zeroImpl(VM& vm)

Does this function really need "impl" in its name?

ALWAYS_INLINE seems like it might be overkill, but I guess it will do no harm.

> Source/JavaScriptCore/runtime/JSBigInt.h:421
> +    static JSValue asIntN(JSGlobalObject*, uint64_t, JSBigInt*);
> +    static JSValue asUintN(JSGlobalObject*, uint64_t, JSBigInt*);
> +#if USE(BIGINT32)
> +    static JSValue asIntN(JSGlobalObject*, uint64_t, int32_t);
> +    static JSValue asUintN(JSGlobalObject*, uint64_t, int32_t);
> +#endif

I don’t think these arguments are really self-explanatory. Maybe give the
uint64_t one a name, and the int32_t one.

Do these really need to be member functions? Why not file-local functions
instead?

> Source/JavaScriptCore/runtime/JSBigInt.h:559
> +    template <typename BigIntImpl>
> +    static ImplResult asIntNImpl(JSGlobalObject*, uint64_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult asUintNImpl(JSGlobalObject*, uint64_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult truncateToNBits(VM&, int32_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult truncateAndSubFromPowerOfTwo(VM&, int32_t, BigIntImpl,
bool resultSign);

Do these really need to be member function templates? Why not file-local
function templates instead?

> Source/JavaScriptCore/runtime/JSBigInt.h:561
> +    static ImplResult zeroImpl(VM&);

Ditto.