[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 a comment.

You are making a pretty good progress!
I think right now there is some code duplication that could be reduced, but otherwise, the checker starts to look really good.



================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:43
+private:
+  bool IsVirtualCall(const CallExpr *CE) const;
+
----------------
This could be maybe a free static function instead of a method?


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:47
+  private:
+    const MemRegion *Region;
+    bool Found;
----------------
Maybe instead of Region the name could be more specific? I.e. the ThisRegion? Or ObjectRegion? 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:101
+    DeclName = CD->getNameAsString();
+    InfoText = "Called from this constrctor " + DeclName;
+  } else {
----------------
In the warning messages we usually enclose the source code snippets (e.g.: declaration names) in single quotes. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:135
+  if (isa<CXXDestructorDecl>(MD)) {
+    auto &SVB = C.getSValBuilder();
+    auto ThiSVal =
----------------
getting the SValBuilder is cheap, maybe you could hoist it from the if statements. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:146
+// The EndFunction callback when leave a constructor or a destructor.
+void VirtualCallChecker::checkEndFunction(CheckerContext &C) const {
+  const auto *LCtx = C.getLocationContext();
----------------
The code of this function is almost completely identical with the checkBegin. I was thinking about abstracting this into a separate function with a bool argument to determine whether we would like to add something to the GDM or remove it. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:186
 
-void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
-  if (ReportPureOnly && !isPure)
+  const auto IC = dyn_cast<CXXInstanceCall>(&Call);
+  if (!IC)
----------------
Maybe you could cast this to CXXMemberCall instead. If that cast succeed, than you do not need to check the Call.getDecl() and also do not need to check for ctor/dtor calls. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:199
+  // The GDM of constructor and destructor should be true.
+  if (IsVirtualCall(CE) && State->get<CtorMap>(Reg)) {
+    if (IsPureOnly && MD->isPure()) {
----------------
Maybe it would be cleaner to early return if the call is not virtual and remove it from these two if statements. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:213
+    } else {
+      ExplodedNode *N = C.generateNonFatalErrorNode();
+      if (!BT)
----------------
Maybe you could extract the error reporting into a separate function with two arguments: the error message, and whether it should generate an error node. 


https://reviews.llvm.org/D34275