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