On Mon, 14 Aug 2023 17:43:09 GMT, Justin Lu <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/text/DigitList.java line 521:
>>
>>> 519: if (non0AfterIndex(maximumDigits)) {
>>> 520: return (isNegative && roundingMode ==
>>> RoundingMode.FLOOR)
>>> 521: || (!isNegative && roundingMode ==
>>> RoundingMode.CEILING);
>>
>> `roundingMode` is checked against `FLOOR`/`CEILING` twice. I don't see the
>> need of bundling them up.
>
> Thank you for the review. Addressed this, and your other comments that I did
> not explicitly respond to.
I'd think this separate one-liner for each case would be simpler and more
readable:
case CEILING:
return nonZeroAfterIndex(maximumDigits) && !isNegative;
case FLOOR:
return nonZeroAfterIndex(maximumDigits) && isNegative;
>> src/java.base/share/classes/java/text/DigitList.java line 543:
>>
>>> 541: && (maximumDigits > 0)
>>> && (digits[maximumDigits - 1] % 2 != 0));
>>> 542: // Not already rounded, and not exact,
>>> round up
>>> 543: } else {
>>
>> Are you sure this logic is equivalent to the previous one? Previously,
>> `HALF_UP/DOWN` checks `valueExactAsDecimal` before `alreadyRounded`, but the
>> new one checks `alreadyRounded` first.
>
> Yes, since `alreadyRounded` and `valueExactAsDecimal` are never both `true`
> and when either of them are `true`, it warrants a return without considering
> other logic.
>
> However, I have adjusted the code so that this is more apparent (and appears
> more similar to the original `HALF_DOWN/UP`, which was written more
> concisely).
OK, thanks for confirming
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1294841815
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1294841883
