[PATCH] D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:822-831
+    assignToContainer(C, CE, LVal, Cont);
+    if (!(LPos = getIteratorPosition(State, LVal)))
       return;
-    State = saveComparison(State, Condition, LVal, RVal, Op == OO_EqualEqual);
-    C.addTransition(State);
-  } else if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
-    if ((State = processComparison(
-             State, getRegionOrSymbol(LVal), getRegionOrSymbol(RVal),
-             (Op == OO_EqualEqual) == (TruthVal->getValue() != 0)))) {
+  } else if (!RPos) {
+    assignToContainer(C, CE, RVal, Cont);
+    if (!(RPos = getIteratorPosition(State, RVal)))
+      return;
----------------
Welcome to the `addTransition()` hell!

Each of the `assignToContainer()` may add a transition, and then `processComparison()` also adds transitions. I suspect that it may lead to more state splits that were intended. I.e., the execution path on which the iterator is assigned to a container would be different from the two execution paths on which the comparison was processed.

You can chain `addTransition()`s to each other, eg.:
```lang=c++
// Return the node produced by the inner addTransition()
ExplodedNode *N = assignToContainer(...);

// And then in processComparison(N, ...)
C.addTransition(N->getState()->assume(*ConditionVal, false), N);
C.addTransition(N->getState()->assume(*ConditionVal, true), N);
```

It should also be possible to avoid transitions until the final state is computed, if the code is easy enough to refactor this way:
```lang=c++
// No addTransition() calls within, just produce the state
ProgramStateRef State = assignToContainer(...);

// And then in processComparison(N, ...)
C.addTransition(State->assume(*ConditionVal, false), N);
C.addTransition(State->assume(*ConditionVal, true), N);
```

This sort of stuff can be tested via `clang_analyzer_numTimesReached()` - see if you made exactly as many state splits as you wanted to.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53701/new/

https://reviews.llvm.org/D53701