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()`.

cfe-commits mailing list

Reply via email to