NoQ added a comment. Other than the packaging debate, i think this check looks good!
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:641 -} // end: "optin.cplusplus" - -let ParentPackage = CplusplusAlpha in { - -def ContainerModeling : Checker<"ContainerModeling">, - HelpText<"Models C++ containers">, - Documentation<NotDocumented>, - Hidden; - -def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">, - HelpText<"Reports destructions of polymorphic objects with a non-virtual " - "destructor in their base class">, - Documentation<HasAlphaDocumentation>; - -def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">, - HelpText<"Check integer to enumeration casts for out of range values">, - Documentation<HasAlphaDocumentation>; - -def IteratorModeling : Checker<"IteratorModeling">, - HelpText<"Models iterators of C++ containers">, +def STLAlgorithmModeling : Checker<"STLAlgorithmModeling">, + HelpText<"Models the algorithm library of the C++ STL.">, ---------------- STL algorithm modeling, as such, doesn't need to be opt-in. That said, as i mentioned, i strongly prefer the state split on `std::find` to be opt-in, because there's usually no obvious indication in the code that the search is expected to fail. Let's move the checker to `cplusplus` and add an option to it ("aggressive std::find modeling" or something like that) that'll enable exploration of the failure branch. I prefer it as an option because there definitely is a chance that we'll actually want it on by default (like we did with the move-checker for locals - it's still wonky but it seems to work really well in practice), and if we do that, i'd prefer not to have to rename a checker. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:27-32 + mutable IdentifierInfo *II_find = nullptr, *II_find_end = nullptr, + *II_find_first_of = nullptr, *II_find_if = nullptr, + *II_find_if_not = nullptr, *II_lower_bound = nullptr, + *II_upper_bound = nullptr, *II_search = nullptr, + *II_search_n = nullptr; + ---------------- Dead code? ================ 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); ---------------- Typo: *ahead. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:138-139 + SVB.getConditionType()); + assert(Less.getAs<DefinedSVal>() && + "Symbol comparison must be a `DefinedSVal`"); + StateFound = StateFound->assume(Less.castAs<DefinedSVal>(), true); ---------------- Is this because you only have atomic conjured symbols in your map? Because otherwise the assertion will fail every time we reach a maximum symbol complexity during `evalBinOp`. I'd rather make the code defensive and handle the `UnknownVal` case. That said, you can be sure it's not `UndefinedVal`. 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