Bug 181144

Created attachment 398037 [details] Patch WIP

Created attachment 398342 [details] Patch

Created attachment 398349 [details] Patch

Comment on attachment 398349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398349&action=review Informal review there. LGTM with one comment regarding test coverage. > Source/JavaScriptCore/runtime/BigIntConstructor.cpp:144 > + JSValue bigInt = toBigInt(globalObject, callFrame->argument(1)); I think we should add test cases that verifies if both `toIndex` and `toBigInt` are being properly called. I think that there are some cases of it on Test262, but it would be nice to add this coverage under JSC stress tests.

Comment on attachment 398349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398349&action=review > Source/JavaScriptCore/ChangeLog:9 > + This patch implements BigInt.asIntN[1] and BigInt.asUintN[2] features. > + As the same to the other BigInt runtime C++ code, we port V8 code to JSC to implement both. Can you describe here what they do?

Comment on attachment 398349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398349&action=review Thanks >> Source/JavaScriptCore/ChangeLog:9 >> + As the same to the other BigInt runtime C++ code, we port V8 code to JSC to implement both. > > Can you describe here what they do? Added >> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:144 >> + JSValue bigInt = toBigInt(globalObject, callFrame->argument(1)); > > I think we should add test cases that verifies if both `toIndex` and `toBigInt` are being properly called. I think that there are some cases of it on Test262, but it would be nice to add this coverage under JSC stress tests. Added

Created attachment 398399 [details] Patch

Comment on attachment 398399 [details] 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.

Comment on attachment 398399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398399&action=review >> 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? Oops, this is replace-error. Fixed. >> 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". number of bits sounds nice. Fixed. Changed to auto too. >> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:145 >> + 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. For integers, toBigInt should throw an error. So it does not matter. For boolean, we already have a fast path in toBigInt, which creates BigInt32 instead of heap JSBigInt. So it does not allocate heap object. I think this is enough efficient for now. >> 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. The name "Impl" is used since this function is returning ImplResult, so, this is zero-impl. If we want to have `JSBigInt::zero(VM&)`, then we should wrap this function, and return JSValue instead. For now, I left ALWAYS_INLINE to make sure this is inlined since it is quite small enough. >> Source/JavaScriptCore/runtime/JSBigInt.h:421 >> +#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? Add names to these variables. They are called from BigIntConstructor, so they need to be exposed as non-file-local functions. >> Source/JavaScriptCore/runtime/JSBigInt.h:559 >> + 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? This is necessary since these functions are reading static private variables / calling private functions of JSBigInt. In addition, we are doing the same thing for the other JSBigInt functions. So doing this is good in terms of consistency. >> Source/JavaScriptCore/runtime/JSBigInt.h:561 >> + static ImplResult zeroImpl(VM&); > > Ditto. zeroImpl can be a static function. Fixed.

Committed r261156: <https://trac.webkit.org/changeset/261156>

<rdar://problem/62879642>

Re-opened since this is blocked by bug 211447

Reverted r261156 for reason: Break ARM64_32 build due to existing bug Committed r261167: <https://trac.webkit.org/changeset/261167>

Created attachment 398519 [details] Patch Patch for relanding

Committed r261199: <https://trac.webkit.org/changeset/261199> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398519 [details].