[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

[Devin Coughlin via Phabricator via cfe-commits]

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