RFR: 8314169: Combine related RoundingMode logic in j.text.DigitList [v2]

[Naoto Sato]

On Mon, 14 Aug 2023 17:43:09 GMT, Justin Lu <jlu at 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