[PATCH] D34275: [analyzer] Re-implemente current virtual calls checker in a path-sensitive way

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:72
+REGISTER_MAP_WITH_PROGRAMSTATE(CtorMap, const MemRegion *, bool)
+REGISTER_MAP_WITH_PROGRAMSTATE(DtorMap, const MemRegion *, bool)
+
----------------
I was wondering if there is another way to represent the state.
We could have a two element (bool based) enum class like:
```
enum class ObjectState : bool {
  CtorCalled,
  DtorCalled
};
```

Se we could have only one map in the GDM. Either the destructor is called for an object or the constructor.  Or in case none of them is called on a path, the state is empty. What do you think? 



================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:86
+  const LocationContext *LCtx = N->getLocationContext();
+  ProgramStateManager &PSM = State->getStateManager();
+  auto &SVB = PSM.getSValBuilder();
----------------
You could move the declarations of PSM and SVB after the next early return. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:107
+
+  std::string DeclName;
+  std::string InfoText;
----------------
You could eliminate this local variable. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:169
 
-  // FIXME: The interprocedural diagnostic experience here is not good.
-  // Ultimately this checker should be re-written to be path sensitive.
-  // For now, only diagnose intraprocedurally, by default.
-  if (IsInterprocedural) {
-    os << "Call Path : ";
-    // Name of current visiting CallExpr.
-    os << *CE->getDirectCallee();
-
-    // Name of the CallExpr whose body is current being walked.
-    if (visitingCallExpr)
-      os << " <-- " << *visitingCallExpr->getDirectCallee();
-    // Names of FunctionDecls in worklist with state PostVisited.
-    for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(),
-         E = WList.begin(); I != E; --I) {
-      const FunctionDecl *FD = (*(I-1))->getDirectCallee();
-      assert(FD);
-      if (VisitedFunctions[FD] == PostVisited)
-        os << " <-- " << *FD;
-    }
+  if (IsPureOnly && !MD->isPure())
+    return;
----------------
These two checks could be moved before you query the CXXThisVal. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:247
+  C.emitReport(std::move(Reporter));
+  return;
 }
----------------
This return is redundant. 


https://reviews.llvm.org/D34275