[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

[Umann Kristóf via Phabricator via cfe-commits]

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