[PATCH] D76361: [Analyzer] Iterator Modeling - Model `std::advance()`, `std::prev()` and `std::next()`

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:221
+    if (Handler) {
+      if (!C.wasInlined) {
+        if (Call.getNumArgs() > 1) {
----------------
Perhaps putting this hunk into a separate function or lambda could decrease the nesting level, b/c you could have an early return if there is no `Handler`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:222
+      if (!C.wasInlined) {
+        if (Call.getNumArgs() > 1) {
+          (this->**Handler)(C, OrigExpr, Call.getReturnValue(),
----------------
Maybe a comment could be helpful here about the common signature of the prev/next functions. Like:
```
advanceFun ( It it, Distance n = 1 )
```
By the way, can we call `std::advance` with one argument?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:238
+      if (IdInfo) {
+        if (IdInfo->getName() == "advance") {
+          if (noChangeInPosition(C, Call.getArgSVal(0), OrigExpr)) {
----------------
Should this be `__advance`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:559
 
+bool IteratorModeling::noChangeInPosition(CheckerContext &C, SVal Iter,
+                                          const Expr *CE) const {
----------------
Some comments would be helpful here. Also, should we use this function only with `advance()` or it could be useful perhaps in other context?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:571
+  const ExplodedNode *N = C.getPredecessor();
+  while (N) {
+    ProgramPoint PP = N->getLocation();
----------------
I have a rough presumption that this hunk is a general pattern to get the previous node for the previous iterator position. So perhaps it would be useful as a standalone free/static member function too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76361