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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/Analysis/CFG.cpp:3899
+  if (auto *CE = const_cast<CXXConstructExpr *>(NE->getConstructExpr()))
+    CurrentConstructionContext = {/*Constructor=*/CE, /*Trigger=*/NE};
+
----------------
dcoughlin wrote:
> 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.
Yes, for now it'd be overwritten every time we meet a new constructor (this wasn't the case in the first version of this revision). However, the additional check (which should be eventually replaced with an assertion, but is for now needed) that we're picking the right context in `CXXConstructExpr` visit, together with only visiting every constructor once during CFG construction during normal visits (because weird stuff like `CXXDefaultArgExpr` isn't yet supported), guarantees that we're not doing anything wrong.

I should have cleaned this up, but i don't want to invest attention into that because subsequent patches will clearly make things way more complex than that, whenever we start dealing with a multitude of construction contexts or with partially-constructed contexts.

So for now it's a trivial kinda-safe solution.

I should document that though, for sure.


================
Comment at: lib/Analysis/CFG.cpp:4388
+        case Stmt::WhileStmtClass: {
+          const VarDecl *var = cast<WhileStmt>(stmt)->getConditionVariable();
+          if (var)
----------------
dcoughlin wrote:
> Please, let's try to avoid changes that are unrelated to the functionality being added.
I decreased indent of this whole huge for() loop, so the blame is already screwed (but phabricator carefully hides that), so i think this is a valid part of the patch.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:632
+
+void ExprEngine::ProcessConstructor(const CFGConstructor C,
+                                    ExplodedNode *Pred) {
----------------
dcoughlin wrote:
> 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?
> 
Hmm, well, yeah, i guess, there isn't much value in passing the `CFGElement` around as long as we can find the current `CFGElement` any time we want in `ExprEngine->currBldrCtx`.

But if not for that, it was the whole point to have the construction context there. Will fix.


https://reviews.llvm.org/D42672