[webkit-reviews] review granted: [Bug 183996] [ESNext][BigInt] Implement support for "/" operation : [Attachment 340267] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 183996: [ESNext][BigInt] Implement support for "/" operation
https://bugs.webkit.org/show_bug.cgi?id=183996

Attachment 340267: Patch

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




--- Comment #31 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 340267
  --> https://bugs.webkit.org/attachment.cgi?id=340267
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSBigInt.cpp:295
> +	       return resultSign == x->sign() ? x : unaryMinus(state, x);

Pass VM& instead.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:298
> +	   absoluteDivWithDigitDivisor(state, x, divisor, &quotient,
remainder);

Pass VM& instead.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:300
> +	   absoluteDivWithBigIntDivisor(state, x, y, &quotient, nullptr);

Pass VM& instead.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:306
> +JSBigInt* JSBigInt::copy(ExecState& state, JSBigInt* x)

Use VM& parameter instead of ExecState&.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:317
> +JSBigInt* JSBigInt::unaryMinus(ExecState& state, JSBigInt* x)

Use VM& parameter instead of ExecState&.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:579
> +inline int JSBigInt::absoluteCompare(JSBigInt* x, JSBigInt* y)

Use ComparisonResult for the resulted value.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:605
> +void JSBigInt::absoluteDivWithDigitDivisor(ExecState& state, JSBigInt* x,
Digit divisor, JSBigInt** quotient, Digit& remainder)

Use VM& parameter instead of ExecState&.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:639
> +void JSBigInt::absoluteDivWithBigIntDivisor(ExecState& state, JSBigInt*
dividend, JSBigInt* divisor, JSBigInt** quotient, JSBigInt** remainder)

Use VM& parameter instead of `ExecState&`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:670
> +	   divisor = absoluteLeftShiftAlwaysCopy(state, divisor, shift,
SameSizeResult);

Pass VM& instead of ExecState&.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:674
> +    JSBigInt* u = absoluteLeftShiftAlwaysCopy(state, dividend, shift,
AlwaysAddOneDigit);

Pass VM& instead of ExecState&.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:802
> +JSBigInt* JSBigInt::absoluteLeftShiftAlwaysCopy(ExecState& state, JSBigInt*
x, unsigned shift, LeftShiftMode mode)

Use `VM&` parameter instead since this function does not require `ExecState*`.

> Source/JavaScriptCore/runtime/JSBigInt.h:97
> +    static JSBigInt* divide(ExecState&, JSBigInt* x, JSBigInt* y);

Should take ExecState* to align to the other functions (multiply). In JSC,
using ExecState* is more common.

> Source/JavaScriptCore/runtime/JSBigInt.h:102
>      enum ComparisonResult {

Use enum class.

> Source/JavaScriptCore/runtime/JSBigInt.h:133
> +    enum LeftShiftMode {
> +	   SameSizeResult,
> +	   AlwaysAddOneDigit
> +    };

Use enum class.