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

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

baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added a comment.

In D70818#1860115 <https://reviews.llvm.org/D70818#1860115>, @Szelethus wrote:

> I dont think any of my comments were addressed -- could you follow up?


I addressed two, added fixme for another one. Now I explained why I did not address the rest.



================
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>;
 
----------------
Szelethus wrote:
> This checker should be `Hidden`.
When this checker gets enabled by default, it should be hidden. However, now nothing depends on it, it is not enabled by default thus making it hidden now is the same as making it nonexistent.


================
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);
----------------
Szelethus wrote:
> 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?
`z` is ahead of `y`. `y` is behind `z`.


================
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);
----------------
Szelethus wrote:
> 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?
FIXME added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70818