[PATCH] D76590: [Analyzer] Model `empty()` member function of containers

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

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:56
+                    SVal RetVal) const;
+  void handleErase(CheckerContext &C, const Expr *CE, SVal Cont, SVal Iter,
+                   SVal RetVal) const;
----------------
martong wrote:
> I understand that the container SVal is no longer `const`, but why did the iterator loose its constness?
We passed it earlier as a constant reference, but `SVal` is small, it should be passed by value.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:277
 void ContainerModeling::handleBegin(CheckerContext &C, const Expr *CE,
-                                   const SVal &RetVal, const SVal &Cont) const {
+                                   SVal RetVal, SVal Cont) const {
   const auto *ContReg = Cont.getAsRegion();
----------------
martong wrote:
> Why do we skip the `const` qualifiers here?
Do you mean for `SVal`? See above.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:436
+    if (!StateEmpty && !StateNonEmpty) {
+      C.generateSink(State, C.getPredecessor());
+      return;
----------------
martong wrote:
> Is there really no sense to continue the analysis here? In what state we are here exactly? Is there an inconsistency between the `begin` , `end` and the return value?
Yes, see the comment I added. If data about emptiness based on the difference of begin and and and the return value of `empty()` contradict each other then we are on an impossible branch.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:19
 
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+                              SymbolRef Sym2, bool Equal);
----------------
martong wrote:
> Docs, docs, docs.
This function is moved here, it is nothing new.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:274
+assumeComparison(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
+                 DefinedSVal RetVal, OverloadedOperatorKind Op) {
+  if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
----------------
martong wrote:
> Perhaps we should guard `Op` with an assert, I mean it should be either == or !=.
Actually, this works for any comparison operator. So it is enough to assume that `Op` is a comparison operator.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:290
+  if (StateTrue)
+    StateTrue = StateTrue->assume(RetVal, true);
+  if (StateFalse)
----------------
martong wrote:
> Why do we `assume` here again? We already had an assume in `relateSymbols`, hadn't we?
In `relateSymbols` we assumed the comparison of the two symbols. Here we assume the `RetVal`. The new states are `nullptr` if we cannot assume the same for both.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:297
+
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+                              SymbolRef Sym2, bool Equal) {
----------------
martong wrote:
> Would `assumeEqual` be a better name?
This method is just moved from another source file.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:301
+
+  // FIXME: This code should be reworked as follows:
+  // 1. Subtract the operands using evalBinOp().
----------------
martong wrote:
> Please explain in the comment what is the problem with the current impl.
Ditto. The community requested a future refactoring.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:313-314
+
+  auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+  if (!NewState)
+    return nullptr;
----------------
martong wrote:
> Could we merge the declaration of the new state into the parens of the `if`?
No, because we use it later in the function. (See below.)


================
Comment at: clang/test/Analysis/container-modeling.cpp:19
 
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+    unsigned int __line, __const char *__function)
----------------
martong wrote:
> It seems like we don't use it anywhere.
We use this in the definition of the `assert()` macro. And we use that macro in the new test cases.


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

https://reviews.llvm.org/D76590