New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8257401: Use switch expressions in jdk.internal.net.http and java.net.http #1364
Conversation
👋 Welcome back amCap1712! A progress list of the required criteria for merging this PR into |
@amCap1712 The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Just a question. |
return "MAX_HEADER_LIST_SIZE"; | ||
} | ||
return "unknown parameter"; | ||
return switch (i + 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kartik. I think it would improve the readability for each of the switch/case expressions if you tab aligned each of the cases, and place the default case on its own line e.g.
return switch (i + 1) {
case HEADER_TABLE_SIZE -> "HEADER_TABLE_SIZE";
case ENABLE_PUSH -> "ENABLE_PUSH";
case MAX_CONCURRENT_STREAMS -> "MAX_CONCURRENT_STREAMS";
case INITIAL_WINDOW_SIZE -> "INITIAL_WINDOW_SIZE";
case MAX_FRAME_SIZE -> "MAX_FRAME_SIZE";
case MAX_HEADER_LIST_SIZE -> "MAX_HEADER_LIST_SIZE";
default -> "unknown parameter";
};
@johnshajiang I don't think we have any global guideline but in this specific case I agree that using switch expressions as proposed is making the code generally more readable, with maybe a few cases where it might not have improved readability as much (for instance I'm not a big fan of line 54 in that file: https://github.com/openjdk/jdk/pull/1364/files#diff-ae0f662913a65511f983a3f0f9a98b708c11906a6f87aa4c96ab6fc7abf5ae9f ) best regards, -- daniel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the actual source changes look good.
A few notes:
- there are whitespace issues. jcheck is failing.
- Please do not force push - just push. Force push messes up prior comments in the thread.
case MAX_FRAME_SIZE -> "MAX_FRAME_SIZE"; | ||
case MAX_HEADER_LIST_SIZE -> "MAX_HEADER_LIST_SIZE"; | ||
|
||
default -> "unknown parameter"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check failure on line 81 in src/java.net.http/share/classes/jdk/internal/net/http/frame/SettingsFrame.java
openjdk / jcheckWhitespace error
Column 0: trailing whitespace
...
WRT to whitespace errors detected by jcheck
, note that you can fix them by running the script:
make/scripts/normalizer.pl
on this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's handy. Thanks!
case HEADER_FOUND_CR, HEADER_FOUND_LF -> resumeOrLF(input); | ||
case HEADER_FOUND_CR_LF -> resumeOrSecondCR(input); | ||
case HEADER_FOUND_CR_LF_CR -> resumeOrEndHeaders(input); | ||
case INITIAL -> state = State.STATUS_LINE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Although, I think you can improve it further if you align the lambda operators as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes right. Just noticed I had missed that. Fixed in latest commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better, Kartik. Thanks
@amCap1712 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 176 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@ChrisHegarty, @dfuch) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@ChrisHegarty Thanks for the review. I'll keep in mind not to use force push again. |
@pconcannon, If everything is in order, can you please approve the changes on Github. I'll then issue the integrate command. Thanks. |
Hi @amCap1712, you will have to /integrate first, and then afterwards I will sponsor |
/integrate |
@amCap1712 |
/reviewer credit @pconcannon |
@amCap1712 |
/sponsor |
@pconcannon @amCap1712 Since your change was applied there have been 176 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit ac54900. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi!
Kindly review this patch to replace switch statements with switch expressions (where it makes sense) in the http client modules. The rationale is to improve readability of the code.
Regards,
Kartik
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1364/head:pull/1364
$ git checkout pull/1364