Simplify ifs with literal conditions even if they jump
#193
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This isn't necessary; the visitor already look for jumps. I thought about keeping it as an optimization, but I don't think it actually optimizes anything since the visitor has to rediscover the jumps anyways.
timtebeek
reviewed
Oct 15, 2023
src/main/java/org/openrewrite/staticanalysis/RemoveUnreachableCodeVisitor.java
Outdated
Show resolved
Hide resolved
Contributor
|
I really like this change. It's concise and very readable. I've added another unit test for the |
pstreef
approved these changes
Oct 23, 2023
Contributor
|
Thank you for the contribution! 🎉 |
src/main/java/org/openrewrite/staticanalysis/RemoveUnreachableCodeVisitor.java
Show resolved
Hide resolved
Member
|
Great work all around! Probably best to run this against the default set through Moderne, just to spot check that it works well there. Well already have an impressive collection of tests here, just looking to make sure we haven't missed anything. |
Contributor
Author
|
Thanks for the feedback and merge, all! 😄 |
Contributor
I ran it, all results look good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed?
This PR improves the
SimplifyConstantIfBranchExecutionrecipe to simplifyifs even if they have jumps (return,throw,break, orcontinue) in the branch we keep. The recipe will now also remove any unreachable code introduced by simplifying theseifs.I added numerous unit tests to demonstrate the new behavior.
What's your motivation?
Fixes #192
Have you considered any alternatives or workarounds?
I considered implementing the unreachable code elimination as another recipe. However, it feels like it's tightly coupled to the
ifsimplification itself; unreachable code is a compile-time error, so if this recipe is going to simplify theseifs, I think it should do so cleanly.Checklist
./gradlew licenseFormat