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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
