On Wed, 9 Jun 2021 16:31:10 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Patrick Concannon has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - 8268469: Removed excessive spacing; corrected misplaced comments >> - Merge remote-tracking branch 'origin/master' into JDK-8268469 >> - Merge remote-tracking branch 'origin/master' into JDK-8268469 >> - 8268469: Update java.time to use switch expressions > > src/java.base/share/classes/java/time/Month.java line 491: > >> 489: case OCTOBER -> 274 + leap; >> 490: case NOVEMBER -> 305 + leap; >> 491: default -> 335 + leap; > > It would be better to keep `DECEMBER` here for clarity - even if only in a > comment - e.g: > > > case NOVEMBER -> 305 + leap; > // otherwise (DECEMBER) > default -> 335 + leap; Hi Daniel. Thanks for your suggestion. Comment added. Please see commit 2ae4a57 > src/java.base/share/classes/java/time/chrono/ThaiBuddhistDate.java line 334: > >> 332: >> 333: default -> with(isoDate.with(field, newValue)); >> 334: }; > > My preference would be to keep the imbricated switch structure: it is not > obvious whether this change is correct. Have you verified that > `getChronology().range(chronoField).checkValidIntValue(newValue, > chronoField);` is a no op when chronoField == ERA? I've reverted these changes as requested. Please see commit 2ae4a57 > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > line 4992: > >> 4990: } >> 4991: default -> throw new >> IllegalStateException("unreachable"); >> 4992: }; > > Not sure I like the new version more than the old, as it seems to lead to > more duplication. > Maybe the 'Y' case should be tested before entering the switch - then the > switch could be used to assign > `var field = switch (chr) {...};` Reverted these changes as requested. Please see commit 2ae4a57 ------------- PR: https://git.openjdk.java.net/jdk/pull/4433