[PATCH] D83190: [analyzer] Model iterator random incrementation symmetrically

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware accepted this revision.
baloghadamsoftware added a comment.
This revision is now accepted and ready to land.

Looks good, aside from the few naming issues I mentioned. Please try it on //LLVM/Clang// before committing it to avoid unexpected crashes.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:112
                               OverloadedOperatorKind Op, const SVal &RetVal,
-                              const SVal &LHS, const SVal &RHS) const;
+                              const SVal &Iterator, const SVal &Offset) const;
   void handlePtrIncrOrDecr(CheckerContext &C, const Expr *Iterator,
----------------
In my subsequent patches I began to use the name `Amount` instead of `Offset` to not confuse with `IteratorPosition::Offset`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:281
+    const Expr *const &IterExpr = IsIterOnLHS ? LHS : RHS;
+    const Expr *const &OtherExpr = IsIterOnLHS ? RHS : LHS;
+
----------------
`AmountExpr`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:385
+        const QualType FstType = FstExpr->getType();
+        const QualType SndType = SndExpr->getType();
+
----------------
`Expr1`, `Expr2`, `Type1`, `Type2` or something similar. `Fst` is to be confused with //Fast// and `Snd` with //Sound//. Or spell out `First` and `Second`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:400
+                 "Either first or second argument should have structure "
+                 "or class type!");
           handleRandomIncrOrDecr(C, OrigExpr, Op, Call.getReturnValue(),
----------------
This is generally true in //C++// that overloaded operators must either be class member or must have at least one class argument. Do we have to assert it in this particular checker?


================
Comment at: clang/test/Analysis/iterator-modeling.cpp:152
 
-void plus(const std::vector<int> &v) {
+void lhs_plus(const std::vector<int> &v) {
   auto i1 = v.begin();
----------------
`plus_lhs`, `plus_rhs` to begin with the name of the operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83190