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

[Sebastian Redl]

Chris Lattner wrote:
>
> 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.
I don't particularly like making even abbreviations all-uppercase in 
CamelCase names, but OK.
>   The name "Guard" is somewhat strange for this.  How about "ASTOwner" 
> or "ASTHolder" or something like that?
I use Guard for most things that guard against early exit from a frame. 
Or sentry, which is a synonym. I didn't use a name like Holder because 
the ExprResult/ExprTy/Stmt equivalents aren't actually part of the class 
- the class is an external addon to guard against early exit, and it 
needs to be told to watch a variable that you already have.
>
> +  public:
> +    enum AstType {
> +      Stmt,
> +      Expr
> +    };
>
> Would it make sense to make an ExprAstGuard and StmtAstGuard class
Yes.
> (or a template parameterized on ExprResult/StmtResult)?
Feels wrong.
> In other words, make this determination statically instead of 
> dynamically?
Yes, it is. I went for the dynamic way because I didn't have any 
immediately obvious idea of what to template on (and specializing 
individual members is a hassle, if at all possible), and I didn't want 
to duplicate the code.
But I think I should just make it

template <void (Action::*Destroyer)(void*)> class ASTGuard;
typedef ASTGuard<&Action::DestroyExpr> ExprGuard;
typedef ASTGuard<&Action::DestroyStmt> StmtGuard;

Then the destructor is

~ASTGuard() { (Actions.*Destroyer)(Node); }

with Node being a new name for AstElement. If the compiler is smart, 
this should be equivalent to a direct function call.
>
> +  // 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?
I suppose that's possible in the actual cases where I use it. I didn't 
before, because I wasn't sure where I would put it. And of course, this 
means making it a template on the smallvector size.

Sebastian