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