On Fri, 11 Aug 2023 18:27:47 GMT, Justin Lu <j...@openjdk.org> wrote:
> Please review this PR which is a broad clean up of the DigitList class (used
> by Format classes in j.text).
>
> This PR is intended to be a portion of a bigger change (split up to make
> reviewing easier).
>
> The main change combines related Rounding Mode logic in `shouldRoundUp()` -
> (_CEILING/FLOOR_, _HALF_UP/DOWN/EVEN_)
>
> Other changes include
> - Certain for loops can be replaced with cleaner syntax (E.g. for(;;), empty
> for loops)
> - Introduce overloaded `round(int)` - For use by Integer representations of
> DigitList
> - Introduce `non0AfterIndex(int)` - To reduce code duplication
src/java.base/share/classes/java/text/DigitList.java line 123:
> 121: * from the given index until the end.
> 122: */
> 123: private boolean non0AfterIndex(int index) {
I'd prefer spelling out `0` to `Zero`.
src/java.base/share/classes/java/text/DigitList.java line 409:
> 407: * Upon return, count will be less than or equal to maximumDigits.
> 408: */
> 409: private void round(int maximumDigits) {
If the method is only used for `Long` and `BigInteger`, probably use the
specific name instead of explaining it in the description and overloading would
be more clear to me.
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.
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
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1291822984
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1291829499
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1291833892
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1291834906
PR Review Comment: https://git.openjdk.org/jdk/pull/15252#discussion_r1291843188
