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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:34
 
-  void handleBegin(CheckerContext &C, const Expr *CE, const SVal &RetVal,
-                   const SVal &Cont) const;
-  void handleEnd(CheckerContext &C, const Expr *CE, const SVal &RetVal,
-                 const SVal &Cont) const;
-  void handleAssignment(CheckerContext &C, const SVal &Cont,
-                        const Expr *CE = nullptr,
-                        const SVal &OldCont = UndefinedVal()) const;
-  void handleAssign(CheckerContext &C, const SVal &Cont) const;
-  void handleClear(CheckerContext &C, const SVal &Cont) const;
-  void handlePushBack(CheckerContext &C, const SVal &Cont) const;
-  void handlePopBack(CheckerContext &C, const SVal &Cont) const;
-  void handlePushFront(CheckerContext &C, const SVal &Cont) const;
-  void handlePopFront(CheckerContext &C, const SVal &Cont) const;
-  void handleInsert(CheckerContext &C, const SVal &Cont,
-                    const SVal &Iter) const;
-  void handleErase(CheckerContext &C, const SVal &Cont, const SVal &Iter) const;
-  void handleErase(CheckerContext &C, const SVal &Cont, const SVal &Iter1,
-                   const SVal &Iter2) const;
-  void handleEraseAfter(CheckerContext &C, const SVal &Cont,
-                        const SVal &Iter) const;
-  void handleEraseAfter(CheckerContext &C, const SVal &Cont, const SVal &Iter1,
-                        const SVal &Iter2) const;
+  void handleBegin(CheckerContext &C, const Expr *CE, SVal RetVal,
+                   SVal Cont) const;
----------------
Perhaps it would be easier to read these functions if we had a documentation for their parameters. Maybe we could have some of them documented in one comment since many seem to have similar params.
Or maybe just renaming `Cont` to `Container` would help also.


================
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;
----------------
I understand that the container SVal is no longer `const`, but why did the iterator loose its constness?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:74
 
-  typedef void (ContainerModeling::*NoItParamFn)(CheckerContext &,
-                                                 const SVal &) const;
+  typedef void (ContainerModeling::*NoItParamFn)(CheckerContext &, const Expr *,
+                                                 SVal, SVal) const;
----------------
Just a nit, if we are at counting below (one, two) then perhaps `Zero` would be more coherent with the other typedefs?


================
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();
----------------
Why do we skip the `const` qualifiers here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:436
+    if (!StateEmpty && !StateNonEmpty) {
+      C.generateSink(State, C.getPredecessor());
+      return;
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:19
 
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+                              SymbolRef Sym2, bool Equal);
----------------
Docs, docs, docs.


================
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>()) {
----------------
Perhaps we should guard `Op` with an assert, I mean it should be either == or !=.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:276
+  if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
+    if ((State = relateSymbols(State, Sym1, Sym2,
+                              (Op == OO_EqualEqual) ==
----------------
Could we spare the `if` with just keeping the declaration of `State` and then
```
return std::make_pair(State, nullptr);
```
?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:277-278
+    if ((State = relateSymbols(State, Sym1, Sym2,
+                              (Op == OO_EqualEqual) ==
+                               (TruthVal->getValue() != 0)))) {
+      return std::make_pair(State, nullptr);
----------------
This is hard to read for me. Also, Isn't it enough just to write `TruthVal->getValue()` ?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:290
+  if (StateTrue)
+    StateTrue = StateTrue->assume(RetVal, true);
+  if (StateFalse)
----------------
Why do we `assume` here again? We already had an assume in `relateSymbols`, hadn't we?


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


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.h:172
                                  long Scale);
+std::pair<ProgramStateRef, ProgramStateRef>
+assumeComparison(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
----------------
Documentation would be helpful.


================
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)
----------------
It seems like we don't use it anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76590