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

[Justin Lu]

On Fri, 11 Aug 2023 22:33:08 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> Justin Lu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Reflect review comments (8/11/23)
>
> 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.

> src/java.base/share/classes/java/text/DigitList.java line 526:
> 
>> 524:             case HALF_UP:
>> 525:             case HALF_DOWN:
>> 526:                 case HALF_EVEN:
> 
> Fix the indentation

Thanks for catching (copy paste error)

> 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).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1293781469
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1293781511
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1293781546