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

Mon Jul 6 12:53:45 PDT 2020

baloghadamsoftware added a comment.

Nice work, but please implement this feature also for non-pointer iterators for the matter of consistence.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:279
+    // or on the RHS (eg.: 1 + it). Both cases are modeled.
+    bool IsItOnLHS = BO->getLHS()->getType()->isPointerType();
+    Expr *&ItExpr = IsItOnLHS ? LHS : RHS;
----------------
gamesh411 wrote:
> What do you think about this detection strategy? This assumes that the Iterator being detected is a pointer (and not a used-defined type like STL iterators etc.). Would you say that this assumption holds every time because the pointer-iterators are only handled in this checkPostStmt callback and the traditional iterators in another callback?
Instead of `It` please use `Iter`, beacause `It` can be understood as the English //it// pronoun: "Is //it// on the left-hand side?"

================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:282
+    SVal &OffsetVal = IsItOnLHS ? RVal : LVal;
+    handlePtrIncrOrDecr(C, ItExpr, BinaryOperator::getOverloadedOperator(OK),
+                        OffsetVal);
----------------
gamesh411 wrote:
> During the development of this patch, I saw something related. At the beginning of handlePtrIncrOrDecr, there is a branch on whether the Expr (2nd argument) is a pointer. I think that branch could just be an assertion. What do you think? (or maybe I should create a patch to show what I mean?)
I wonder whether this should be implemented here in `checkPostStmt()` ot in `handlePtrIncrOrDecr()`. Your current implementation is in `checkPostStmt()`, in this case we can assert in `handlePtrIncrOrDecl()`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83190