-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8367499: Refactor exhaustiveness computation from Flow into a separate class #27253
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
base: pr/27247
Are you sure you want to change the base?
Conversation
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
.collect(Collectors.toSet()); | ||
|
||
for (PatternDescription pdOne : patterns) { | ||
if (pdOne instanceof BindingPattern bpOne) { |
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.
The scope of this if
is quite wide, you could consider inverting the condition and continue
ing if the condition does not match. This would save an indentation level.
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.
Sorry, but the point here is to move the code from one place to another (as suggested in the description). I suspect that any change, despite how desirable it might look, is likely to make reviewing this change more difficult, and hence I would like to avoid that.
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 see, thanks for the clarification.
@lahodaj this pull request can not be integrated into git checkout JDK-8367499
git fetch https://git.openjdk.org/jdk.git pr/27247
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/27247"
git push |
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.
lgtm, looks clean
Currently the switch exhaustiveness computation code is part of
Flow
. And while conceptually the check is part of theFlow
phase, the code is >500 lines of code currently, and likely to get bigger/more complicated in the future. Among other reasons due to enhancements like https://bugs.openjdk.org/browse/JDK-8367530.The proposal herein is to move the exhaustiveness computation to a separate class
ExhastivenessComputer
. There's no functional change, only move of the code. This is intentional, to aid the review process.One possibility to inspect what is happening is:
and inspecting
/tmp/exhaustivenesscomputer-comparison.diff
,/tmp/flow-comparison.diff
.Progress
Integration blocker
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27253/head:pull/27253
$ git checkout pull/27253
Update a local copy of the PR:
$ git checkout pull/27253
$ git pull https://git.openjdk.org/jdk.git pull/27253/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27253
View PR using the GUI difftool:
$ git pr show -t 27253
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27253.diff
Using Webrev
Link to Webrev Comment