[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Adam,
This looks like a nice improvement. I have some remarks inline.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:382
     }
+  } else if (!isa<CXXConstructorCall>(&Call)) {
+    // The main purpose of iterators is to abstract away from different
----------------
The function becomes > 100 lines long. Should we refactor this check into a separate function to improve readability?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:388
+    // template parameters for different containers. So we can safely
+    // assume that passing iterators of different containers as arguments
+    // whose type replaces the same template parameter is a bug.
----------------
While this assumption is sane and is true for <algorithm> functions, user code can have other design solutions. There is nothing that prevents users from writing a function looking like:
```
template <typename IterTy>
void f(IterTy FromBegin, IterTy FromEnd, IterTy ToBegin, IterTy ToEnd);
```
and there is nothing wrong with it.
One of  possible solutions is to restrict checker to check only functions from std namespace. What do you think?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:410
+    // Iterate over all the template parameters
+    for (auto i = 0U; i < TParams->size(); ++i) {
+      const auto *TPDecl = dyn_cast<TemplateTypeParmDecl>(TParams->getParam(i));
----------------
size_t I = 0?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:924
+    }
+    reportMismatchedBug("Iterator access mismatched.", Iter1, C, N);
+  }
----------------
We always report about first iterator, but the mismatched one can be second. I think this deserves a FIXME, at least.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1025
 
+void IteratorChecker::reportMismatchedBug(const StringRef &Message,
+                                          const SVal &Val, CheckerContext &C,
----------------
We usually pass StringRefs and SVals by value because they're very cheap for copying. However, the surrounding code follows the same conventions so it's not strongly required to change.


https://reviews.llvm.org/D32845