On Fri, 15 Nov 2024 14:21:21 GMT, fabioromano1 <d...@openjdk.org> wrote:
>> After changing `BigInteger.sqrt()` algorithm, this can be also used to speed
>> up `BigDecimal.sqrt()` implementation. Here is how I made it.
>>
>> The main steps of the algorithm are as follows:
>> first argument reduce the value to an integer using the following relations:
>>
>> x = y * 10 ^ exp
>> sqrt(x) = sqrt(y) * 10^(exp / 2) if exp is even
>> sqrt(x) = sqrt(y*10) * 10^((exp-1)/2) is exp is odd
>>
>> Then use BigInteger.sqrt() on the reduced value to compute the numerical
>> digits of the desired result.
>>
>> Finally, scale back to the desired exponent range and perform any adjustment
>> to get the preferred scale in the representation.
>
> fabioromano1 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Optimize sqrt branch for exact results
src/java.base/share/classes/java/math/BigDecimal.java line 2248:
> 2246: }
> 2247: }
> 2248: } else { // mc.roundingMode == RoundingMode.UP ||
> mc.roundingMode == RoundingMode.CEILING
It would be more robust to make all enum constant explicit, and reserve the
`else` to throw a `RuntimeException` of some sort, e.g., `MatchException`.
Should an additional enum constant be added to `RoundingMode` in the future,
this code will then throw instead of silently producing a wrong result.
If possible at all, it would be preferable to use a `switch` rather than `if`s.
src/java.base/share/classes/java/math/RoundingMode.java line 425:
> 423: default -> false;
> 424: };
> 425: }
For this PR, it would be preferable to move this method as a private static in
BigDecimal itself, even if its usefulness is more broad.
Also, I suggest making all the cases explicit, like so
Suggestion:
private static boolean isHalfWay(RoundingMode m) {
return switch (m) {
case HALF_DOWN, HALF_UP, HALF_EVEN -> true;
case FLOOR, CEILING, DOWN, UP, UNNECESSARY -> false;
};
}
In this way, should another enum constant be added in the future, the method
will throw instead of silently returning a wrong value, indicating that a new
case must be added to the switch for it to be exhaustive. (There's a hidden
`default` that throws.)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1846658170
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1846627164
