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

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 2 11:19:23 PST 2018


NoQ added inline comments.


================
Comment at: lib/Analysis/CFG.cpp:3899
+  if (auto *CE = const_cast<CXXConstructExpr *>(NE->getConstructExpr()))
+    CurrentConstructionContext = {/*Constructor=*/CE, /*Trigger=*/NE};
+
----------------
NoQ wrote:
> 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.
Fixed now - added the `VisitForConstructionContext()` wrapper which contains the assertion that checks that we're not overwriting any existing context.

The context is being cleaned up after it's consumed in `appendConstructor()`.


https://reviews.llvm.org/D42672





More information about the cfe-commits mailing list