Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Thu, 17 Jun 2021 13:51:27 GMT, Daniel Fuchs 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/format/DateTimeFormatterBuilder.java
> line 2793:
>
>> 2791: case NOT_NEGATIVE-> throw new
>> DateTimeException("Field " + field +
>> 2792: " cannot
>> be printed as the value " + value +
>> 2793: " cannot
>> be negative according to the SignStyle");
>
> Maybe in this case you could revert the alignment. In this case it would
> allow lines 2792 to 2793 to be shorter
I've removed the alignment of arrow operators as requested. See 24ae9e5
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 10:58:22 GMT, Stephen Colebourne
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/LocalDate.java line 607:
>
>> 605: if (field instanceof ChronoField chronoField) {
>> 606: if (chronoField.isDateBased()) {
>> 607: int n = switch (chronoField) {
>
> This logic is harder to read than before, and it requires the CPU to perform
> an extra branch on `(n == -1)`. It should just be a `return switch ...`
Reverted logic as requested. See 24ae9e5
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Thu, 17 Jun 2021 13:56:00 GMT, Daniel Fuchs wrote: >> It is your codebase, not mine, so it is up to you. Aligning things by column >> is generally frowned on in most style guides because it handles refactoring >> poorly, resulting in lots of needless change (or people forgetting to >> realign things - I had to deal with a rogue aligned Javadoc signature today >> in a PR where exactly that had happened). I also don't see how you write a >> style guide rule for when these should be aligned and when they should not. >> >> Anyway, this PR isn't really the right place for this discussion - I'm not >> blocking the PR on this basis (and I'm not an official Reviewer anyway). > > I have a slight preference for the aligned version in the cases where the > lines are short and the number of spaces used for padding is not > unreasonable. I find the code gains in readability in these cases. In the > case where the lines are long - I agree with Stephen that the alignment > doesn't bring much readability - and sometime it may even be detrimental as > it makes long lines longer. Stephen, would that be an acceptable compromise? Its certainly a reasonable position to take (ie. if thats what OpenJDK wants to do, its fine by me). I'm more interested to see how you would write such a thing down in the coding standards that doesn't make the standard worthless. - PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 16:43:20 GMT, Stephen Colebourne wrote: >> The vertical alignment improves readability in these short-line cases. >> Removing the spaces before the arrows will make it a little harder to >> discern the difference between the cases. > > It is your codebase, not mine, so it is up to you. Aligning things by column > is generally frowned on in most style guides because it handles refactoring > poorly, resulting in lots of needless change (or people forgetting to realign > things - I had to deal with a rogue aligned Javadoc signature today in a PR > where exactly that had happened). I also don't see how you write a style > guide rule for when these should be aligned and when they should not. > > Anyway, this PR isn't really the right place for this discussion - I'm not > blocking the PR on this basis (and I'm not an official Reviewer anyway). I have a slight preference for the aligned version in the cases where the lines are short and the number of spaces used for padding is not unreasonable. I find the code gains in readability in these cases. In the case where the lines are long - I agree with Stephen that the alignment doesn't bring much readability - and sometime it may even be detrimental as it makes long lines longer. Stephen, would that be an acceptable compromise? - PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 10:57:07 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.time`
>> packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> 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/format/DateTimeFormatterBuilder.java line
2793:
> 2791: case NOT_NEGATIVE-> throw new
> DateTimeException("Field " + field +
> 2792: " cannot be
> printed as the value " + value +
> 2793: " cannot be
> negative according to the SignStyle");
Maybe in this case you could revert the alignment. In this case it would allow
lines 2792 to 2793 to be shorter
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 11:11:30 GMT, Chris Hegarty wrote:
>> src/java.base/share/classes/java/time/Month.java line 480:
>>
>>> 478: int leap = leapYear ? 1 : 0;
>>> 479: return switch (this) {
>>> 480: case JANUARY -> 1;
>>
>> Unnecessary alignment
>
> The vertical alignment improves readability in these short-line cases.
> Removing the spaces before the arrows will make it a little harder to discern
> the difference between the cases.
It is your codebase, not mine, so it is up to you. Aligning things by column is
generally frowned on in most style guides because it handles refactoring
poorly, resulting in lots of needless change (or people forgetting to realign
things - I had to deal with a rogue aligned Javadoc signature today in a PR
where exactly that had happened). I also don't see how you write a style guide
rule for when these should be aligned and when they should not.
Anyway, this PR isn't really the right place for this discussion - I'm not
blocking the PR on this basis (and I'm not an official Reviewer anyway).
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 9 Jun 2021 22:11:59 GMT, Stephen Colebourne 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/chrono/ThaiBuddhistDate.java line 331: > >> 329: yield with(isoDate.withYear(nvalue - >> YEARS_DIFFERENCE)); >> 330: } >> 331: case ERA -> with(isoDate.withYear((1 - >> getProlepticYear()) - YEARS_DIFFERENCE)); > > `checkValidIntValue` performs validation, so removing it has changed the > behavoiur Changes reverted. See 2ae4a57 - PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 10:59:59 GMT, Stephen Colebourne
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 480:
>
>> 478: int leap = leapYear ? 1 : 0;
>> 479: return switch (this) {
>> 480: case JANUARY -> 1;
>
> Unnecessary alignment
The vertical alignment improves readability in these short-line cases. Removing
the spaces before the arrows will make it a little harder to discern the
difference between the cases.
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 10:57:07 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.time`
>> packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> 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/LocalDate.java line 607:
> 605: if (field instanceof ChronoField chronoField) {
> 606: if (chronoField.isDateBased()) {
> 607: int n = switch (chronoField) {
This logic is harder to read than before, and it requires the CPU to perform an
extra branch on `(n == -1)`. It should just be a `return switch ...`
src/java.base/share/classes/java/time/LocalDateTime.java line 1181:
> 1179: if (unit instanceof ChronoUnit chronoUnit) {
> 1180: return switch (chronoUnit) {
> 1181: case NANOS -> plusNanos(amountToAdd);
Still has unnecessary alignment
There are lots of cases in the PR where `->` is still aligned. I'd like to see
all of them unaligned.
src/java.base/share/classes/java/time/Month.java line 480:
> 478: int leap = leapYear ? 1 : 0;
> 479: return switch (this) {
> 480: case JANUARY -> 1;
Unnecessary alignment
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 9 Jun 2021 16:25:55 GMT, Naoto Sato 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/LocalDateTime.java line 1188:
>
>> 1186: case HOURS -> plusHours(amountToAdd);
>> 1187: case HALF_DAYS -> plusDays(amountToAdd /
>> 256).plusHours((amountToAdd % 256) * 12);
>> 1188: default -> with(date.plus(amountToAdd, unit), time);
>> // no overflow (256 is multiple of 2)
>
> The comment is misplaced.
Hi Naoto. Thanks for spotting this. I've corrected that now. Please see commit
2ae4a57
> src/java.base/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
> line 310:
>
>> 308: case HOURS -> plusHours(amountToAdd);
>> 309: case HALF_DAYS -> plusDays(amountToAdd /
>> 256).plusHours((amountToAdd % 256) * 12);
>> 310: default -> with(date.plus(amountToAdd, unit), time); //
>> no overflow (256 is multiple of 2)
>
> Misplaced comment here too.
Comment corrected. Please see commit 2ae4a57
> src/java.base/share/classes/java/time/format/SignStyle.java line 127:
>
>> 125: boolean parse(boolean positive, boolean strict, boolean fixedWidth)
>> {
>> 126: return switch (ordinal()) {
>> 127: // valid if negative or (positive and lenient)
>
> The comment should apply only to the `case 0`.
Comment corrected. Please see commit 2ae4a57
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 9 Jun 2021 16:31:10 GMT, Daniel Fuchs 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
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
> Hi, > > Could someone please review my code for updating the code in the `java.time` > packages to make use of the switch expressions? > > Kind regards, > Patrick 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4433/files - new: https://git.openjdk.java.net/jdk/pull/4433/files/2abbea11..2ae4a574 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4433=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4433=01-02 Stats: 12287 lines in 307 files changed: 9481 ins; 1521 del; 1285 mod Patch: https://git.openjdk.java.net/jdk/pull/4433.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4433/head:pull/4433 PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 9 Jun 2021 22:03:54 GMT, Stephen Colebourne
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/LocalDate.java line 608:
>
>> 606: if (chronoField.isDateBased()) {
>> 607: return switch (chronoField) {
>> 608: case DAY_OF_MONTH -> ValueRange.of(1,
>> lengthOfMonth());
>
> For the record, I hate code that is aligned like this. It makes refactoring
> much more noisy, as the author has to readjust all the associated lines. I
> also don't believe it is within the spirit of Java's traditional coding
> guidelines.
Hi Stephen. Thanks for your comments. I've tried removing the excessive spacing
as you suggested. Please let me know what you think. See commit 2ae4a57
> src/java.base/share/classes/java/time/format/SignStyle.java line 129:
>
>> 127: // valid if negative or (positive and lenient)
>> 128: case 0 -> !positive || !strict;// NORMAL
>> 129: case 1, 4 -> true; // ALWAYS, EXCEEDS_PAD
>
> Extra space before `4`
Extra space removed. Please see commit 2ae4a57
-
PR: https://git.openjdk.java.net/jdk/pull/4433
