-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
C++ SimpleRangeAnalysis: remove float bounds #16862
base: main
Are you sure you want to change the base?
Conversation
@@ -5,7 +5,7 @@ | |||
* @kind path-problem | |||
* @problem.severity warning | |||
* @security-severity 8.6 | |||
* @precision high | |||
* @precision medium |
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.
Reducing the precision of a query from high
to medium
means it's no longer running as part of Code Scanning. In the past we've not done such drastic changes, so I don't think we should do that as part of this PR.
If you're concerned with the new FP on the query then we could put a barrier on all dataflow nodes with a floating point types in the configuration here.
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.
Ok, you can check out the latest commit.
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.
Awesome! That LGTM 🎉
f4da707
to
fd21a72
Compare
If this doesn't break any tests, then this will simplify the rewrite of all SimpleRangeAnalysis floats to BigInt.
We are mostly losing precision due to excluding float operations from range analysis.
This leads to two regressions, but this should be OK if we decide to drop float support from SimpleRangeAnalysis Keeping high query precision to keep it in code scanning.
d4a2c1e
to
6589fc5
Compare
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.
Changes LGTM! I think we should consider adding a minorAnalysis
library change note saying something like:
"The SimpleRangeAnalysis
library no longer produces type bounds for expressions with floating-point types" (or something like that).
2da7703
to
b123901
Compare
This small change should simplify the SimpleRangeAnalysis library down the line by ignoring floats, but it also reduces the precision of some queries.
Some coding standard tests are also expected to fail for the same reason. So I'll leave it to the C/C++ team's discretion whether to move forward in this direction.
Commits: