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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

First of all, thanks everybody for working on this. I'd see what i can do.



================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:29
 namespace {
+enum class ObjectState : bool { CtorCalled, DtorCalled };
+} // end namespace
----------------
Please remind me what was wrong with ascending over `StackFrameContext`s to see if we're inside a constructor or destructor call? Why did we need a state trait here? Was it too hard to obtain this-values? (Not sure if there's time to fix this, but it might be worth it to leave a FIXME around).


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:56
+                                   CheckerContext &C) const;
+  void reportbug(StringRef Msg, bool PureError, const MemRegion *Reg,
+                 CheckerContext &C) const;
----------------
I wish for camelCase.


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:110
+  auto ThiSVal =
+      State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame()));
+  const MemRegion *Reg = ThiSVal.getAsRegion();
----------------
I don't think this is working. CXXThisRegion is never a region of a C++ object; it's the segment of the stack where the pointer is passed, you need to dereference its value to get the actual object region.

Probably tests for the visitor might make things more clear.


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:194
+    if (IsPureOnly && MD->isPure())
+      reportbug("Call to pure function during construction", true, Reg, C);
+    else
----------------
"Pure function" doesn't mean what we want to say here (https://en.wikipedia.org/wiki/Pure_function), i think we should stick with "pure virtual function" anyways.


================
Comment at: test/Analysis/virtualcall.cpp:15
     foo();
-#if INTERPROCEDURAL
-        // expected-warning-re at -2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction has undefined behavior}}
-#endif
+        // expected-warning:Call to virtual function during construction
   }
----------------
Tests need fixing: `expected-warning` requires `{{` and needs to be on the correct line.


https://reviews.llvm.org/D34275