[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Looks good with minor comments.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:493
+    for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+      SR.markLive(*i);
   }
----------------
Let's only mark `SymbolData`-type symbols as live; that should be enough.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1168
+  assert(comparison.getAs<DefinedSVal>() &&
+    "Symbol comparison must by a `DefinedSVal`");
+
----------------
Typo: "be".


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1170-1177
+  auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+  if (const auto CompSym = comparison.getAsSymbol()) {
+    assert(isa<SymIntExpr>(CompSym) &&
+           "Symbol comparison must be a `SymIntExpr`");
+    assert(BinaryOperator::isComparisonOp(
+               cast<SymIntExpr>(CompSym)->getOpcode()) &&
+           "Symbol comparison must be a comparison");
----------------
I believe this code should be reworked as follows:

1. Subtract the operands using `evalBinOp()`.
2. Assume that the result doesn't overflow.
3. Compare the result to 0.
4. Assume the result of the comparison.

This should hopefully yield the same result without ever needing to introspect the `SymExpr`.

I suggest a FIXME.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1237
+  assert(comparison.getAs<DefinedSVal>() &&
+    "Symbol comparison must by a `DefinedSVal`");
 
----------------
Also typo.


https://reviews.llvm.org/D48764