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

Aleksei Sidorin via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 16 05:45:45 PST 2018

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.


More information about the cfe-commits mailing list