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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
