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

[Artem Dergachev via Phabricator via cfe-commits]

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