Skip to content

C++: Implement guards logic for switch statements #15958

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 19, 2024
Merged

C++: Implement guards logic for switch statements #15958

merged 9 commits into from
Mar 19, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Mar 18, 2024 •
edited
Loading

This PR is the follow-up to #15941 that addresses this comment I made in that PR:

It's still possible to do more in this area. For example, GuardCondition.comparesEq still doesn't reason about switch statements since the interface on that predicate can't really support it (its columns include (Expr left, Expr right, int k) which holds if left == right + k, but the case label in a switch isn't an expression). So such changes are probably best done in a follow-up PR.

Commit-by-commit review recommended.

This PR only does the required API additions to support GuardCondition.comparesEq for switch statements. We also have a predicate GuardCondition.comparesLt which needs a similar treatment. I'm delaying that until a future PR

@github-actions github-actions bot added the C++ label Mar 18, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 18, 2024
View reviewed changes
@MathiasVP MathiasVP force-pushed the ir-guards-from-switch-statements-2 branch from 094e1be to cb78f31 Compare March 18, 2024 13:52
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 18, 2024
View reviewed changes
@MathiasVP MathiasVP force-pushed the ir-guards-from-switch-statements-2 branch from a56b5cd to 40dbc6f Compare March 18, 2024 16:27
@MathiasVP MathiasVP marked this pull request as ready for review March 19, 2024 09:29
@MathiasVP MathiasVP requested a review from a team as a code owner March 19, 2024 09:29
@jketema
Copy link
Contributor

jketema commented Mar 19, 2024

I see some alert changes in DCA. Are those correct?

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Mar 19, 2024 •
edited
Loading

I see some alert changes in DCA. Are those correct?

I thought they were correct, but now that I look slightly closer I've spotted a problem. Thanks for making me double check them! I've fixed in this 350b239. Rerunning DCA now.

@MathiasVP
Copy link
Contributor Author

I should've paid more attention to the test changes. They revealed the same problem >.<

jketema
jketema approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if DCA is happy this time around

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Mar 19, 2024 •
edited
Loading

DCA looks uneventful now (🎉 I guess 😄)

@MathiasVP MathiasVP merged commit 597f008 into github:main Mar 19, 2024
This was referenced Mar 19, 2024
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jketema jketema jketema approved these changes

Assignees
No one assigned
Labels
C++ documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MathiasVP @jketema