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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

LGTM! Minor nitpicking in comments.

Currently there's no such problem, but as `GetRange` becomes more complicated, we'll really miss the possibility of saying something like "if there's a range for negated symbol, `return GetRange(the negated symbol)`", so that all other special cases applied. We could have allowed that by canonicalizing symbols (i.e. announce that `$A` always goes before `$B` and therefore we will store a range for `$A - $B` even if we're told to store the range for `$B - $A`) and then the "if" will become "if the symbol is not canonical, `return GetRange(the canonicalized symbol)`".



================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:546
 
+  if (const SymSymExpr *SSE = dyn_cast<SymSymExpr>(Sym)) {
+    // FIXME: Once SValBuilder supports unary minus, we should use SValBuilder
----------------
Can we move the whole `if` into a function?

Eg.,
```
if (RangeSet *R = getRangeForMinusSymbol(Sym))
  return R->Negate(BV, F)
```
?


================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:557
+                                              SSE->getLHS(), T);
+      if (ConstraintRangeTy::data_type *negV =
+          State->get<ConstraintRange>(negSym)) {
----------------
`ConstraintRangeTy::data_type` ~> `RangeSet` should be easier to read.


https://reviews.llvm.org/D35110