Skip to content

C++: Cleanup missingGuardAgainstOverflow #5780

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

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 27, 2021

Now that #5678 is merged we can undo the ugly hack I added in bb447d7 to detect when exprMightOverflowPositively (and Negatively) couldn't analyze the expression.

When doing this change, I realized that we forgot to include the assignment versions of arithmetic operations (and left shifts) in the set of expressions that might overflow. I have added these in 05d693e.

We remove a false positive from our tests now because sc1 += 0 was analyzable (and not overflowing), so convertedExprMightOverflowPositively didn't hold. But because the other disjunct (i.e., the hacky one I added in bb447d7):

upperBound(e.getFullyConverted()) = exprMaxVal(e.getFullyConverted())

was satisfied, we still marked this expression as possible overflowing. Now that we can just use convertedExprMightOverflowPositively, we remove this false positive.

@jbj
Copy link
Contributor

jbj commented Apr 27, 2021

Lots of test failures in header-variant-tests. They seem unrelated.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@geoffw0 geoffw0 merged commit afa8925 into github:main Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants