Szelethus added a comment.
Makes sense!
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:39
// currently treated as an lvalue.
// * type-IIc, type-IIIc: compound values of iterator-typed objects, when the
// iterator object is treated as an rvalue taken of a
----------------
Did you mean lazy compound values?
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:174
-// Structure fo recording iterator comparisons. We needed to retrieve the
-// original comparison expression in assumptions.
-struct IteratorComparison {
+// Structure fo recording symbol comparisons. We need to retrieve the original
+// comparison expression in assumptions.
----------------
typo: for
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:332-333
BinaryOperator::Opcode getOpcode(const SymExpr *SE);
-const RegionOrSymbol getRegionOrSymbol(const SVal &Val);
-const ProgramStateRef processComparison(ProgramStateRef State,
- RegionOrSymbol LVal,
- RegionOrSymbol RVal, bool Equal);
-const ProgramStateRef saveComparison(ProgramStateRef State,
- const SymExpr *Condition, const SVal
&LVal,
- const SVal &RVal, bool Eq);
-const IteratorComparison *loadComparison(ProgramStateRef State,
- const SymExpr *Condition);
+const ProgramStateRef saveComparison(ProgramStateRef State, SymbolRef
Condition,
+ SymbolRef LSym, SymbolRef RSym, bool Eq);
+const SymbolComparison *loadComparison(ProgramStateRef State,
----------------
I think this name would be better if you added the new state to
`CheckerContext` within this function (and making it `void`), or rename it to
`getEvaluatedComparisonState`.
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:373
SymbolRef NewSym, SymbolRef CondSym, BinaryOperator::Opcode Opc);
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+ SymbolRef Sym2, bool Equal);
----------------
I think `evaluateComparison` would be a more fitting name.
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1197-1199
while (const auto *CBOR = Cont->getAs<CXXBaseObjectRegion>()) {
Cont = CBOR->getSuperRegion();
}
----------------
You will have to excuse me for commenting on something totally unrelated, but
I'm afraid this may cause a crash, if the region returned by `getSuperRegion`
is symbolic. I encountered this error when doing the exact same thing in my
checker: D50892. Can something like this occur with this checker?
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2057-2058
auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+ if (!NewState)
+ return nullptr;
+
----------------
It isn't obvious to me (at first) what happens here -- maybe document when this
function will return with `nullptr`? When `relateSymbol` is called and checked
whether the returned value is null or not, one could think that this symbolizes
some sort of failure.
Repository:
rC Clang
https://reviews.llvm.org/D53701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
