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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
