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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:382
     }
+  } else if (!isa<CXXConstructorCall>(&Call)) {
+    // The main purpose of iterators is to abstract away from different
----------------
a.sidorin wrote:
> The function becomes > 100 lines long. Should we refactor this check into a separate function to improve readability?
Yes, I think so this would be a good idea. Should I do it now?


================
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.
----------------
a.sidorin wrote:
> 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?
We can restrict, of course, but first we should measure how it performs on real code. With the restriction, we can get rid of some false positives but we may also loose some true positives.


https://reviews.llvm.org/D32845