JDK-8264222: Use switch expression in jshell where possible #3210
Conversation
|
👋 Welcome back tvaleev! A progress list of the required criteria for merging this PR into |
Webrevs
|
briangoetz
left a comment
briangoetz
left a comment
There was a problem hiding this comment.
Cute trick for the partial update to c.
The comment in the last hunk (resultTypeOf()) only applied to the INSTANCE and STATIC init cases, but now it applies to CONSTRUCTOR too (in a previous hunk, you deliberately had a redundant case, to preserve this information.) I would update the comment.
Otherwise, all looks good to me, +1.
|
@amaembo This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 20 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
… position the comments properly
|
@briangoetz thank you for review!
I updated the comment and moved it before the case branch. Please take a look.
Yeah, IDEA does this automatically! |
|
I have some formatting questions. I also have some weak opinions on formatting, but not strong enough to make recommendations yet. (I see you wrote "Better formatting ideas are welcome" so thanks for being open to this.) I'm interested in feedback on how the code ends up reading, particularly from the people who will be maintaining this code. But comments from others are welcome too. The switch (c) in In (1) Here's the modified version as proposed in the PR: (2) Variation with the arrows on the same line, but lined up: (3) Variation with arrows on the next line: (4) Variation with arrows on the same line, aligned, with case constants one per line: Personally I like variations (2) and (4), since they not only line the arrows up, the column of arrows creates a visual divider between the case constants and the values. Variation (3) is quite visually noisy to my eye, though still probably an improvement over the original code (a switch statement, not expression). Are there other variations? What do people think? |
|
I'll answer from two perspectives: what I personally like, vs what I
would recommend in a style guide.
From a personal perspective, I like #2 the best, and #4 after that. It
is really easy to see what's going on. But, cases where you can
actually line up like this are rare, since most switches involve more
than just simple constants on the RHS.
On the other hand, I would never ask anyone to code like that; it seems
too obsessive to ask people to do. What I'd suggest from a style guide
perspective is #1.
For this particular example, #3 is the worst on both points.
But, this particular example is specific; both the predicates and
consequences are syntactically short. If they were longer, #3 would be
more attractive.
So as a general recommendation #1 if it fits, #3 if #1 doens't fit, and
#2/#4 if the formatting works out and you're feeling generous.
…On 3/26/2021 8:40 PM, Stuart Marks wrote:
I have some formatting questions. I also have some weak opinions on
formatting, but not strong enough to make recommendations yet. (I see
you wrote "Better formatting ideas are welcome" so thanks for being
open to this.) I'm interested in feedback on how the code ends up
/reading/, particularly from the people who will be maintaining this
code. But comments from others are welcome too.
The switch (c) in |ArgTokenizer::nextToken| is pretty nicely
formatted, since each case has a single constant, they're all short,
and the values are also all constants that are textually short. The
only thing I'd say here is to add a bit of spacing to the default case
so that the arrows line up.
In |Eval::prettyExpr| switch (typeName), the switch is a bit irregular
since one of the case arms has multiple constants. This makes it a bit
harder to find one if you're looking for it in particular. For
example, suppose you want to find out how "int" is handled. In the old
code you can scan down looking at each |case| and then look at the
constant right next to it. In the new code (as modified) the "int" is
farther away from the |case| keyword and is a bit lost among the
values, so it's harder to see. Let me see how the variations look.
Here's the modified version as proposed in the PR:
|String sinit = switch (typeName) { case "byte", "short", "int" ->
"0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" ->
"0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default
-> "null"; }; |
Variation with the arrows on the same line, but lined up:
|String sinit = switch (typeName) { case "byte", "short", "int" ->
"0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" ->
"0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default
-> "null"; }; |
Variation with arrows on the next line:
|String sinit = switch (typeName) { case "byte", "short", "int" ->
"0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" ->
"0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default
-> "null"; }; |
Variation with arrows on the same line, aligned, with case constants
one per line:
|String sinit = switch (typeName) { case "byte", "short", "int" ->
"0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" ->
"0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default
-> "null"; }; |
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3210 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJ4RHKGGO7PSHNFZSZD6DTFUSOZANCNFSM4Z2NQ5BQ>.
|
|
Right, with longer case constants or values, the formatting dynamics of the entire expression change. Here's another example from the changeset, from (5) Here's the version as proposed in the PR: (6) Line breaks before the arrow: (7) An extra blank line between each case: (8) As above, but with the case constants one to a line: To my eye, (5) is too dense. The lines are too long. This is indented two levels (one for method in class, the second for code in method) and the longest line is 116 characters long. (Personally I tend to try to keep lines close to 100 or so at most.) The density of the lines makes it difficult to pick out the case constants from the values, as the arrows are buried in the middle. It's impossible to align the arrows, since that would make the lines even longer. (6) puts line breaks before each arrow, which is kind of like (3) from the previous round of examples. This is an improvement, I think, and it's tolerable, whereas in the previous round (3) was quite bad. It's still quite dense, however. For (7) I simply added line breaks between each case. I think this actually improves things a lot. It still takes up less vertical space than the original switch statement, but the additional white space makes it easier to read, and to pick out the case constants from values, I think. I note that in a switch statement, since (8) is a variation of (7) with the case constants on individual lines. The advantage here is that all the constants line up with each other, but frankly I don't think this buys much. It also creates a weird disjointed indent, where the second and subsequent case constants are indented by five because of C-A-S-E-space, but the arrow is at the standard indent of four. You could line up the arrows at an indent level of five, but bleargh, this doesn't really help anything. ===== From my first round of examples, I said I liked (2) and (4), but revisiting them, I've decided I like (4) less. I suppose I could tolerate (1) but the constants and values being all smashed together makes it difficult to separate them. I'd recommend (2) more strongly because having the arrows all line up provides a clear visual separation between the constants and the values. Of course, the recommendation for (2) stands only if the constants and values can all fit on a single line. For the second round of examples, I think adding the blank lines makes a big difference. It means that the switch expression takes up almost as much vertical space as the switch statement. It can save a bit because with the statement, the multiple case labels were each on their own line, whereas for the expression I'm ok with putting the constants on the same line. Even though almost as much vertical space is consumed, I think the readability is quite good, and it still gains the benefits of switch expressions like exhaustiveness and lack of fall-thru. |
|
(5) is fine if it fits within reasonable line limits, and is what we
should recommend as the starting point.
(6) is the natural way to unwrap (5) when the lines get long.
(Personally I'm largely indifferent to 6/7/8; they are all acceptable,
and I think the distinction is mostly based on how dense and nonuniform
the consequences are.)
If the consequences are "similar" to each other, or they are simple,
then 6 is a winner. If they start to diverge in subtle ways, adding
more whitespace may make it easier for readers to understand what is
going on, which might pull us towards 7/8.
Like with the previous choices, I might be inclined to do 7 for myself,
but I wouldn't want to require others to code that way. I think its in
the category of "use more whitespace if you think it makes the code more
readable."
So:
- 5 if it fits
- 6 if you need a little more space
- 7 or 8 if (6) results in something too dense to read
One thing I think you've omitted, but which we should cover, is whether
the -> goes on the line with the case, or the consequence:
case FOO ->
fooAction()
vs
case FOO
-> fooAction()
I think this is similar to advice for joining two long boolean
expressions with &&, or chains of method invocations; it is more obvious
that the second line is a continuation of the first when the operator is
at the left margin rather than the right. So I prefer the second to the
first (as all your examples used.)
…On 3/27/2021 1:11 AM, Stuart Marks wrote:
Right, with longer case constants or values, the formatting dynamics
of the entire expression change. Here's another example from the
changeset, from |Eval::kindOfTree|.
(5) Here's the version as proposed in the PR:
|OuterWrap outer = switch (probableKind) { case IMPORT ->
state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip); case
EXPRESSION ->
state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR, TYPE_DECL, METHOD ->
state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default ->
state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource)); }; |
(6) Line breaks before the arrow:
|OuterWrap outer = switch (probableKind) { case IMPORT ->
state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip); case
EXPRESSION ->
state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR, TYPE_DECL, METHOD ->
state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default ->
state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource)); }; |
(7) An extra blank line between each case:
|OuterWrap outer = switch (probableKind) { case IMPORT ->
state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip); case
EXPRESSION ->
state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR, TYPE_DECL, METHOD ->
state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default ->
state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource)); }; |
(8) As above, but with the case constants one to a line:
|OuterWrap outer = switch (probableKind) { case IMPORT ->
state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip); case
EXPRESSION ->
state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR, TYPE_DECL, METHOD ->
state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default ->
state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource)); }; |
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3210 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJ4REFPSB5USLVAJZ2AADTFVSG7ANCNFSM4Z2NQ5BQ>.
|
|
@briangoetz @stuart-marks thank you for your comments. I don't like (2) because the first case is significantly longer than the other ones. As a result, there's too much of whitespace on the subsequent lines, and it's hard to follow visually from the case label to the resulting expression without the editor's help (e. g. highlighting the current line). (4) solves this problem, so for code reading, it looks the best. There's another consideration though: the code maintenance cost. I'm not sure about other editors but AFAIK, IntelliJ IDEA doesn't help you to maintain the vertical alignment for switch cases (we have a similar option for variable declarations, probably we should support switch cases as well), so any edits of the surrounding code may require manual reformatting, and autoformatting accidentally applied to this code may screw the things up. As for 5-8, there's also an option to align arrows vertically: (9) OuterWrap outer = switch (probableKind) {
case IMPORT -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);
case EXPRESSION -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR, TYPE_DECL, METHOD -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
};In this case, the longest line has 114 characters, which looks within the surrounding code style. OuterWrap outer = switch (probableKind) {
case IMPORT -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);
case EXPRESSION -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
case VAR,
TYPE_DECL,
METHOD -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
default -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
};By the way, I found that there's a redundant "preview" warning suppression at line 230. If we remove it and inline the variable, there's another opportunity for an expression switch. I also changed to (4) and (10) and added a space after 'default' in |
openjdk
bot
removed
ready
openjdk
bot
added
ready
|
/integrate |
openjdk
bot
added
integrated
|
@amaembo Since your change was applied there have been 81 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 3997c99. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
This change is powered by IntelliJ IDEA quick-fix.
I also took the liberty to fix C-style arrays in changed files.
In SourceCodeAnalysisImpl.java:501, I left case PARAMETERIZED_TYPE -> FALSE as a separate switch case, because it had a comment. Better formatting ideas are welcome.
If you like this, I can provide similar PR in other components as well (e.g. in javac)
Progress
Issue
Reviewers
Download
To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3210/head:pull/3210$ git checkout pull/3210To update a local copy of the PR:
$ git checkout pull/3210$ git pull https://git.openjdk.java.net/jdk pull/3210/head