On Fri, 11 Aug 2023 22:33:08 GMT, Naoto Sato <na...@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
