[cfe-dev] Ownership of Stmts and Exprs, and destroying them

[Chris Lattner]

On Nov 24, 2008, at 12:13 PM, Sebastian Redl wrote:

> Chris Lattner wrote:
>>
>> On Nov 22, 2008, at 3:36 PM, Sebastian Redl wrote:
>>> Is this correct? Because I want to clean this up. I want to make the
>>> Sema routines (and later the parser, too) not leak. So I need to  
>>> know
>>> exactly how this works.
>>
>> This would be great.  Probably the first place to start is to use  
>> audit the parser for places where early exits cause parsed stmts to  
>> be leaked.
>
> OK, I've audited the parser. The result is attached. No regressions,  
> and a clean valgrind run on Parse/statements.c. (Not leak-free. Just  
> invalid-access-free.)
>
> Not all places are fixed. In particular, array bounds and default  
> arguments hidden inside a Declarator might be leaked in some places.  
> Also, the additional guard is not pretty - ideally, ActionResult  
> itself should be replaced by the smart pointer. This is more of a  
> patch than a thorough solution.

Cool!

the Parser.cpp part is independent and can be committed separately  
(and looks fine)

+  class AstGuard {

AST is a contraction, so it should be capitalized.  The name "Guard"  
is somewhat strange for this.  How about "ASTOwner" or "ASTHolder" or  
something like that?

+  public:
+    enum AstType {
+      Stmt,
+      Expr
+    };

Would it make sense to make an ExprAstGuard and StmtAstGuard class (or  
a template parameterized on ExprResult/StmtResult)?  In other words,  
make this determination statically instead of dynamically?

+  // RAII guard for freeing SmallVectors of Stmts and Exprs on early  
exit
+  // in the parser.
+  class AstArrayGuard {

instead of having a pointer to a smallvector, how about just making  
this contain the temporary smallvector?

Since these things affect the rest of the patch, I'm going to wait to  
review the rest until we converge on the API.

-Chris