[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok, code makes sense to me now!

I think we still need a few new tests to cover the corner cases.

In https://reviews.llvm.org/D35110#1135306, @baloghadamsoftware wrote:

> I added extra assertion into the test for the difference. Interestingly, it also works if I assert `n-m` is in the range instead of `m-n`. However, I wonder how can I apply such constraint to the difference of iterator positions without decomposing them to symbols and constants.


I don't see how iterator checker is different from the tests. The code of the program in your tests doesn't decompose `m - n` into symbols and constants, it simply subtracts an opaque value `n` (whatever it is) from an opaque value `m` (whatever it is) and makes assumptions on the opaque result of the subtraction (whatever it is). The checker should do the same, i guess?



================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:177-178
 
+// Turn all [A, B] ranges to [-B, -A]. Turn minimal signed value to maximal
+// signed value.
+RangeSet RangeSet::Negate(BasicValueFactory &BV, Factory &F) const {
----------------
I guess the comment needs to be updated.


================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:189
+        newRanges.begin()->From().isMinSignedValue()) {
+      const llvm::APSInt &newFrom = newRanges.begin()->From();
+      newRanges =
----------------
I suggest a few sanity checks here (untested):
`assert(newRanges.begin()->To().isMinSignedValue());` because we shouldn't ever get an overlap.
`assert(!from.isMinSignedValue())` for the same reason; it's good to know because it tells us what `newTo` is equal to on this path.


https://reviews.llvm.org/D35110