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

I do not see any test cases for this patch. Could you add them as well?

Are you sure that this representation is ok? 
How do you handle the following case?

  struct A {
    A() {
      X x;
      x.virtualMethod(); // this virtual call is ok
    }
  }
  int main() {
    A a;
  }



================
Comment at: VirtualCallChecker.cpp:44
+  private:
+    const unsigned Flag;
+    bool Found;
----------------
The name of this flag is not descriptive enough. Please choose a name that refers to what are you using this value for.


================
Comment at: VirtualCallChecker.cpp:79
+  ProgramStateRef state = N->getState();
+  const unsigned ctorflag = state->get<ConstructorFlag>();
+  const unsigned dtorflag = state->get<DestructorFlag>();
----------------
Variable names should start with uppercase letter.


================
Comment at: VirtualCallChecker.cpp:83
+  const CXXConstructorDecl *CD =
+        dyn_cast<CXXConstructorDecl>(LCtx->getDecl());
+  const CXXDestructorDecl *DD =
----------------
Are you sure that the LCtx->getDecl can not return null? 


================
Comment at: VirtualCallChecker.cpp:114
+                                      CheckerContext &C) const {
+  const Decl *D = dyn_cast_or_null<Decl>(Call.getDecl());
+  if (!D)
----------------
Do you need this cast here?


================
Comment at: VirtualCallChecker.cpp:119
+  ProgramStateRef state = C.getState();
+  const unsigned ctorflag = state->get<ConstructorFlag>();
+  const unsigned dtorflag = state->get<DestructorFlag>();
----------------
Querying the state is not free, I think you should also query something from the state once you are sure that you will need that.


================
Comment at: VirtualCallChecker.cpp:158
+  // GDM of constructor and destructor. 
+  if (isVirtualCall(CE) && ctorflag > 0 && state->get<ObjectFlag>() == 0) {
+    if (!BT_CT) {
----------------
I don't think this is the right approach. Once you increased the ObjectFlag on a path, you will never report anything on that path anymore. I think it might be better to have a map from Symbols (representing this) to ctor/dtors or some other approach. 


================
Comment at: VirtualCallChecker.cpp:260
+  if (SFC->inTopFrame()) {
+  const FunctionDecl *FD = SFC->getDecl()->getAsFunction();
+    if (!FD)
----------------
The formatting seems to be off here I recommend using clang format.


https://reviews.llvm.org/D34275