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