dcoughlin added a comment.
Maxim, thanks for commandeering the patch. I apologize for the long delay
reviewing!
I really like the approach of retrying without scopes enabled when we hit a
construct we can't handle yet. This will make is possible to turn the feature
on by default (and get it running on large amounts of code) while still
developing it incrementally!
I'd like to suggest a change in the scope representation that will make it much
easier to support all the corner cases and also re-use the existing logic for
handling variable scopes. Right now the CFGScopeBegin and CFGScopeEnd elements
use a statement (a loop or a CompoundStatement) to uniquely identify a scope.
However, this is a problem because a `for` loop with an initializer may have
two scopes that we really want to distinguish:
for (int i = 0; i < 10; i++)
int j = i;
Here `i` and `j` really have different scopes, but with the current approach we
won't be able tell them apart.
My suggestion is to instead use the first VarDecl declared in a scope to
uniquely identify it. This means, for example, that CFGScopeBegin's constructor
will take a VarDecl instead of a statement.
What's nice about this VarDecl approach is that the CFGBuilder already has a
bunch of infrastructure to correctly track local scopes for variables (see the
`LocalScope` class in CFG.cpp and the `addLocalScopeForStmt()` function.)
Further, there are two functions, addLocalScopeAndDtors() and
addAutomaticObjHandling() that are already called in all the places where a
scope should end. This means you should only need to add logic to add an end of
scope element in those two functions. What's more, future work to extend CFG
construction to handle new C++ language features will also use these two
functions -- so we will get support for those features for free. For an example
of how to change those two functions, take a look at Matthias Gehre's commit in
https://reviews.llvm.org/D15031. What you would need to do for CFGScopeEnd
would be very similar to that patch. Using this approach should also make it
possible to re-use the logic in that patch to eventually handle gotos.
To handle CFGScopeBegin, you would only need to change
CFGBuilder::VisitDeclSubExpr() to detect when the variable being visited is the
first variable declared in its LocalScope. If so, you would append the
CFGScopeBegin element. What's nice about this is that you won't have to handle
all the different kinds of loops individually.
What do you think? I'd be happy to have a Skype/Google hangouts talk about this
if you want to have real-time discussion about it.
Repository:
rL LLVM
https://reviews.llvm.org/D16403
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
