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;
----------------
NoQ wrote:
> 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.
My feel is that a better `.addTransition()` API should capture the user's
intent more straightforwardly, so that we could check dynamically that the
resulting topology is indeed exactly what the user expects.
I.e., produce multiple narrow-purpose APIs for common patterns:
`C.updateState(State)`, `C.splitState(State1, State2, ..., StateN)` - both
would fail if there were previous transitions in the same `CheckerContext` or
if more transitions are made after them. The `updateState()` variant should
probably try to lazily collapse multiple updates into a single node. Maybe
instead don't require all branches to be specified simultaneously, i.e. instead
do `addBranch(State)` that wouldn't fail in presence of other branches but
would still conflict with `updateState()`.
These narrow-purpose APIs are too clumsy to cover the current use-case, but at
least they would've caught the problem. Maybe a better design could make it
also comfortable to use.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53701/new/
https://reviews.llvm.org/D53701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
