[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a subscriber: NoQ.
xazax.hun added a comment.

I think this change is very useful but it is also important to get these changes right.
I think one of the main reason you did not get review comments yet is that it is not easy to verify that these changes are sound.

In general, there are false positives in the analyzer due to limits in the constraint manager (or missing parts in modeling the language). But in general, we try to avoid having false positives due to unsound assumptions (apart from some cases like assuming const methods will not change the fields of a class).

While the change you introduced is indeed very useful the soundness probably depends on the details of how promotions, conversions, and other corner cases are handled.
In order to introduce a change like this, we need to have those cases covered to ensure that we have the soundness we want and this needs to be verified with test cases.
Also once the solution is sound it would be great to measure the performance to ensure that we did not regress too much.

I understand that you do not want to work on something that might not get accepted but also with the available information it might be hard to decide whether this is a good approach to the problem or not. 
But of course, I am just guessing here, @dcoughlin, @zaks.anna, @NoQ  might have a different opinion.

A bit more technical comment: did you consider using `SValBuilder`'s `evalBinOpNN`? I believe it already handles at least some of the conversions you did not cover here.


Repository:
  rL LLVM

https://reviews.llvm.org/D36471