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

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied 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 340161: Patch

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




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

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

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

Let's make this function `inline`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:522
> +    if (x->length())
> +	   ASSERT(x->digit(0));
> +
> +    if (y->length())
> +	   ASSERT(y->digit(0));

Make them `ASSERT(!x->length() || x->digit(0))` and `ASSERT(!y->length() ||
y->digit(0))`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:-482
> -    int length = x->length();

What "small" means in absoluteDivSmall? DigitDivisor would be better. So,
absoluteDivWithDigitDivisor.

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

Ditto, please name it more descriptive one. I'm not sure what "absolute" means
here.
And what "large" means? `absoluteDivWithBigIntDivisor`?.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:680
> +bool JSBigInt::productGreaterThan(Digit factor1, Digit factor2, Digit high,
Digit low)

Make this function `inline` since it is very small helper function.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:742
> +JSBigInt* JSBigInt::absoluteSpecialLeftShift(ExecState& state, JSBigInt* x,
unsigned shift, SpecialLeftShiftMode mode)

What "Special" means? Please name it more descriptive one.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1093
> +JSBigInt::Digit JSBigInt::digit(unsigned n)

Keep `inline`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1099
> +void JSBigInt::setDigit(unsigned n, Digit value)

Keep `inline`.