[PATCH] D61816: [CFG] [analyzer] pr41300: Add a branch to skip virtual base initializers when they are handled by the superclass.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

I do not like to have a variable storing/mention one stuff named in plural. It is your decision as it is  just my personal feeling.



================
Comment at: clang/include/clang/Analysis/AnalysisDeclContext.h:462
                              bool markElidedCXXConstructors = true,
+                             bool addVirtualBasesBranches = true,
                              CodeInjector *injector = nullptr);
----------------
`Bases` -> `Base`


================
Comment at: clang/include/clang/Analysis/CFG.h:510
+    /// constructor.
+    VirtualBasesBranch,
 
----------------
The most derived class instead. `Bases` -> `Base`


================
Comment at: clang/include/clang/Analysis/CFG.h:541
+    return getKind() == VirtualBasesBranch;
+  }
 };
----------------
`Bases` -> `Base`


================
Comment at: clang/include/clang/Analysis/CFG.h:1050
     bool MarkElidedCXXConstructors = false;
+    bool AddVirtualBasesBranches = false;
 
----------------
`Bases` -> `Base`


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:120
+  void HandleVirtualBasesBranch(const CFGBlock *B, ExplodedNode *Pred);
+
 private:
----------------
`Bases` -> `Base`


================
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:73
     bool addRichCXXConstructors, bool markElidedCXXConstructors,
-    CodeInjector *injector)
+    bool addVirtualBasesBranches, CodeInjector *injector)
     : Injector(injector), FunctionBodyFarm(ASTCtx, injector),
----------------
`Bases` -> `Base`


================
Comment at: clang/lib/Analysis/CFG.cpp:1435
+  // For C++ constructor add initializers to CFG. Virtual base classes may have
+  // already been initialized by the superclass. Make a branch so that it was
+  // possible to jump straight to non-virtual bases and fields, skipping
----------------
The most derived class instead.


================
Comment at: clang/lib/Analysis/CFG.cpp:1794
+    // TODO: Add a VirtualBasesBranch to see if the superclass is responsible
+    // for destroying them.
     const CXXRecordDecl *CD = VI.getType()->getAsCXXRecordDecl();
----------------
`Bases` -> `Base`


================
Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:40
+          /*addVirtualBasesBranches=*/true,
+          injector),
       Ctx(ASTCtx), Diags(diags), LangOpts(ASTCtx.getLangOpts()),
----------------
`Bases` -> `Base`


================
Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:447
 
+void CoreEngine::HandleVirtualBasesBranch(const CFGBlock *B,
+                                          ExplodedNode *Pred) {
----------------
`Bases` -> `Base`


================
Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:451
+  if (const auto *CallerCtor = dyn_cast_or_null<CXXConstructExpr>(
+          LCtx->getStackFrame()->getCallSite())) {
+    switch (CallerCtor->getConstructionKind()) {
----------------
May it is worth to handle the call outside to see whether it is non-null.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:435
+    const auto *OuterCtor = dyn_cast_or_null<CXXConstructExpr>(
+        LCtx->getStackFrame()->getCallSite());
+    assert(
----------------
May it is worth to handle the call outside to see whether it is non-null.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:440
+         OuterCtor->getConstructionKind() == CXXConstructExpr::CK_Delegating) &&
+        ("This virtual base should have been initialized by the superclass!"));
+    (void)OuterCtor;
----------------
The most derived class?


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

https://reviews.llvm.org/D61816