Skip to content

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

Merged
MathiasVP merged 9 commits into
github:mainfrom
MathiasVP:ir-guards-from-switch-statements-2
Mar 19, 2024
Merged

C++: Implement guards logic for switch statements#15958
MathiasVP merged 9 commits into
github:mainfrom
MathiasVP:ir-guards-from-switch-statements-2

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

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
Comment thread cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Fixed
@MathiasVP
MathiasVP force-pushed the ir-guards-from-switch-statements-2 branch from 094e1be to cb78f31 Compare March 18, 2024 13:52
Comment thread cpp/ql/lib/semmle/code/cpp/ir/implementation/EdgeKind.qll Fixed
@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

jketema commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

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

@MathiasVP

MathiasVP commented Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

@jketema jketema left a comment

Copy link
Copy Markdown
Contributor

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

MathiasVP commented Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

DCA looks uneventful now (🎉 I guess 😄)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants