[PATCH] D42672: [CFG] [analyzer] Heavier CFGConstructor elements.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

I think it will be great to have a more explicit representation of construction in the CFG! Comments in line.



================
Comment at: include/clang/Analysis/CFG.h:143
+  CXXConstructExpr *Constructor;
+  Stmt *Trigger;
+
----------------
I think it would be good to add a comment saying what Trigger is.


================
Comment at: include/clang/Analysis/CFG.h:155
+/// constructor expression.
+class CFGConstructor : public CFGElement {
+public:
----------------
Can you add a comment saying that this is only used by the analyzer's CFG?


================
Comment at: lib/Analysis/CFG.cpp:475
 
+  ConstructionContext CurrentConstructionContext = {nullptr, nullptr};
+
----------------
A comment here would be helpful to future maintainers. For example, what does it mean for a construction context to be "Current"?


================
Comment at: lib/Analysis/CFG.cpp:3899
+  if (auto *CE = const_cast<CXXConstructExpr *>(NE->getConstructExpr()))
+    CurrentConstructionContext = {/*Constructor=*/CE, /*Trigger=*/NE};
+
----------------
Is it possible that there is already a CurrentConstructionContext? Will this overwrite it?

Can you write an assertion to make sure that this doesn't happen?

Generally I would expect a context to have a well-defined start and end. I think we should be explicit about where we expect it to start and end so we can find violations of our expectations.


================
Comment at: lib/Analysis/CFG.cpp:4388
+        case Stmt::WhileStmtClass: {
+          const VarDecl *var = cast<WhileStmt>(stmt)->getConditionVariable();
+          if (var)
----------------
Please, let's try to avoid changes that are unrelated to the functionality being added.


================
Comment at: lib/StaticAnalyzer/Core/AnalysisManager.cpp:32
                 /*addCXXNewAllocator=*/true,
+                /*addRichCXXConstructors=*/true,
                 injector),
----------------
I think this really needs to be a flag in AnalyzerOptions so you can keep the existing CFG tests for the behavior that other clients of the CFG rely on and add new tests with this flag explicitly set.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:632
+
+void ExprEngine::ProcessConstructor(const CFGConstructor C,
+                                    ExplodedNode *Pred) {
----------------
You don't seem to be using the constructor context for anything other than getting the CXXConstructExpr. Will a later patch require having a CFGConstructor here?

This seems like unnecessarily duplicated code. Can we just change ProcessStmt to take a `Stmt *` rather than a CFGStmt and use it for all statements including constructors?



https://reviews.llvm.org/D42672