On Sun, 27 Jul 2025 08:26:59 GMT, Thomas Zimmermann <[email protected]> wrote:
>> fabioromano1 has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> A zero root's shift can't be excluded
>
> src/java.base/share/classes/java/math/BigInteger.java line 2773:
>
>> 2771: */
>> 2772: public BigInteger nthRoot(int n) {
>> 2773: return n == 1 ? this : (n == 2 ? sqrt() :
>> nthRootAndRemainder(n, false)[0]);
>
> I'm not a reviewer, but I noticed that inlining the `needRemainder`-flag into
> the callers arguably yields clearer flow, obviates the "Assume n != 1 && n !=
> 2" javadoc and removes the array allocation for `nthRoot`:
>
>
> public BigInteger nthRoot(int n) {
> if (n == 1)
> return this;
>
> if (n == 2)
> return sqrt();
>
> checkNthRoot(n);
> MutableBigInteger[] rootRem = new
> MutableBigInteger(this.mag).nthRootRem(n);
> return rootRem[0].toBigInteger(signum);
> }
>
> public BigInteger[] nthRootAndRemainder(int n) {
> if (n == 1)
> return new BigInteger[] { this, ZERO };
>
> if (n == 2)
> return sqrtAndRemainder();
>
> checkNthRoot(n);
> MutableBigInteger[] rootRem = new
> MutableBigInteger(this.mag).nthRootRem(n);
> return new BigInteger[] {
> rootRem[0].toBigInteger(signum),
> rootRem[1].toBigInteger(signum)
> };
> }
>
> private static void checkNthRoot(int n) {
> if (n <= 0)
> throw new ArithmeticException("Non-positive root degree");
>
> if ((n & 1) == 0 && this.signum < 0)
> throw new ArithmeticException("Negative radicand with even root
> degree");
> }
>
>
> What do you think?
> @zimmi @fabioromano1 I'm fine with this change. However, I wonder if it might
> be beneficial to avoid computing the remainder downstream when invoking
> `nthRoot(int n)`, returning `null` in the array.
Why do useless work to cast the remainder to `BigInteger` when it is not
needed, considering that it could cost an allocation of a new `int[]` array?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24898#discussion_r2236387303
