[code cleanup] rework C++ AST representation of cleanups

[llvmbot]

Bugzilla Link	5646
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@lattner,@DougGregor,@tkremenek,@rjmccall

Extended Description

Currently some semantic information on when cleanups should be run is not present in the AST, and clients must infer it. This is fragile and easy to get wrong.

Doug and I discussed this and came up with some concrete ideas, which are unfortunately dependent on a number of other changes:

1. Use an explicit CleanupScopeStmt. Variables which need to have destructors run are defined to have them run at the end of the containing CleanupScopeStatement, in FIFO order. Clients are still responsible for dealing with the fact that scopes may be exited at funny points (exceptions), and the stack based nature of cleanups.

Temporaries with extended lifetimes are cleaned up at the end of the containing CleanupScopeStmt.

2. Make explicit in the AST where cleanups of temporary objects can occur. The idea is to have Sema use an explicit FullExpr object in contexts where cleanups may need be run.

​2 would be a non-Expr and non-Stmt, which implies some other changes which we need to do anyway:

3. Add Stmt and Expr specific non-mutable iterators/visitors. Switch most clients to use them.

4. Switch the Rewriter off of using a mutable iterator. This may involve providing new good APIs for rewriting ASTs, which, as it turns out, is a good idea anyway.

5. I personally think we should add an explicit ExprStmt, but opinions might vary.

Doug, is this an accurate summary?

[lattner]

Ok! There is no priority associated with this, just though it might be partially addressed. Thanks.

[rjmccall]

I have not done any of these; IR gen still relies heavily on recomputed knowledge of object lifetimes. The use of RAII objects to collect these scopes mitigates the "easy to get wrong" concerns, though.

[lattner]

John, did you do this?

[DougGregor]

Currently some semantic information on when cleanups should be run is not present in the AST, and clients must infer it. This is fragile and easy to get wrong.

Doug and I discussed this and came up with some concrete ideas, which are unfortunately dependent on a number of other changes:

1. Use an explicit CleanupScopeStmt. Variables which need to have destructors run are defined to have them run at the end of the containing CleanupScopeStatement, in FIFO order.

LIFO order.

Clients are still responsible for dealing with the fact that scopes may be exited at funny points (exceptions), and the stack based nature of cleanups.

For this reason, it might make sense for CleanupScopeStmt not to have an actual list of those variables that have destructors. Its presence implies that any variable that requires destruction and is in that scope (but not a nested CleanupScopeStmt, naturally) will be destroyed when the cleanup scope exits.

Temporaries with extended lifetimes are cleaned up at the end of the containing CleanupScopeStmt.

2. Make explicit in the AST where cleanups of temporary objects can occur. The idea is to have Sema use an explicit FullExpr object in contexts where cleanups may need be run.

​2 would be a non-Expr and non-Stmt, which implies some other changes which we

need to do anyway:

As part of this, CXXExprWithTemporaries should disappear entirely. Instead, the FullExpr will store the actual Expr* along with an optional list of temporaries to be destroyed.

3. Add Stmt and Expr specific non-mutable iterators/visitors. Switch most clients to use them.

We might be able to do #​3 independently.

4. Switch the Rewriter off of using a mutable iterator. This may involve providing new good APIs for rewriting ASTs, which, as it turns out, is a good idea anyway.

We don't know what this will involve, because we don't know how much the Rewriter leans on mutability.

5. I personally think we should add an explicit ExprStmt, but opinions might vary.

I agree about having an ExprStmt. I find the Expr -> Stmt inheritance to be awkward.

• Doug