[PATCH] D70818: [Analyzer] Model STL Algoirthms to improve the iterator checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:644
   Dependencies<[ContainerModeling]>,
-  Documentation<NotDocumented>,
-  Hidden;
-
-def InvalidatedIteratorChecker : Checker<"InvalidatedIterator">,
-  HelpText<"Check for use of invalidated iterators">,
-  Dependencies<[IteratorModeling]>,
-  Documentation<HasAlphaDocumentation>;
-
-def IteratorRangeChecker : Checker<"IteratorRange">,
-  HelpText<"Check for iterators used outside their valid ranges">,
-  Dependencies<[IteratorModeling]>,
-  Documentation<HasAlphaDocumentation>;
-
-def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
-  HelpText<"Check for use of iterators of different containers where iterators "
-           "of the same container are expected">,
-  Dependencies<[IteratorModeling]>,
-  Documentation<HasAlphaDocumentation>;
-
-} // end: "alpha.cplusplus"
+  Documentation<NotDocumented>;
 
----------------
This checker should be `Hidden`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:37-38
+
+  typedef bool (STLAlgorithmModeling::*FnCheck)(CheckerContext &,
+                                                const CallExpr *) const;
+
----------------
Prefer `using`. D67706#inline-614013


================
Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:67
+public:
+  STLAlgorithmModeling() {}
+
----------------
` = default;`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:126
+  // assume that in case of successful search the position of the found element
+  // is ahed of it.
+  const auto *Pos = getIteratorPosition(State, SecondParam);
----------------
NoQ wrote:
> Typo: *ahead.
Hold on, isn't it the other way around?

```
[_|_|x|_|_|_|_|y|_|_|_|z|_]
```
Suppose in the range `[x, z)` I'm looking for `y`. The range-end argument would be `z`, isn't `y` behind it? The following code and the test cases seem to be correct, so I guess its the comment?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:129-130
+  if (Pos) {
+    StateFound = createIteratorPosition(StateFound, RetVal, Pos->getContainer(),
+                                        CE, LCtx, C.blockCount());
+    const auto *NewPos = getIteratorPosition(StateFound, RetVal);
----------------
What if the range-end iterator is a reverse iterator? Wouldn't it mess with the relative position of the found element?

```
[_|_|x|_|_|_|_|y|_|_|_|z|_]

std::find(z, x, y);
```

Suppose I'm searching in the `(x, z]` range for `y`, where `x`,  `z` are reverse iterators. Here, you'll create a forward iterator for `y`, if I'm not mistaken, and you'd say that its behind `x`? Are these things correctly modeled in IteratorChecker?


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

https://reviews.llvm.org/D70818