[cfe-commits] r49142 - in /cfe/trunk: include/clang/AST/TranslationUnit.h lib/AST/TranslationUnit.cpp

[Ted Kremenek]

On Apr 7, 2008, at 8:28 AM, Sam Bishop wrote:

> Ted,
>
> Ted Kremenek wrote:
>> So I think the solution is not technically all that hard; it just
>> requires some tedious engineering to make separate instances of all
>> the mentioned methods (and to of course add the necessary "Allocator"
>> argument to the Decl::Create methods, etc.).
>
> Because nearly all of this patch will just consist of adding the new
> method argument, I want to make sure that I'm adding the right  
> argument.
> I have started down the "Allocator" path you suggested, but now I'm  
> not
> sure that's the way to go.
>
> If the argument is an allocator object, I think we would want a  
> typedef.
> "llvm::MallocAllocator" is a little long, and it exposes an
> implementation detail.  In that case, I would want some input  
> regarding
> what to call it and where to put it.  (Is the allocator going to be  
> used
> for more than just Decl objects eventually?  It looks like we'd need a
> new header file just for the typedef, etc.)
>
> Though I agree that making the argument an ASTContext object seems
> sub-optimal at first, I'm starting to lean that direction.  It would
> make the Create and CreateImpl methods symmetrical for the Decl  
> classes.
> The Type CreateImpl methods already require the ASTContext object.
> Chris has indicated elsewhere in this thread that he's planning  
> Destroy
> methods which will also take an ASTContext.  What do you think?
>
> Thanks,
> Sam Bishop
>

Hi Sam,

You've convinced me.

Adding an ASTContext argument to the Create/CreateImpl methods for  
Decl (and its subclasses) makes sense.  I double checked the logic in  
TranslationUnit::Create, and ASTContext is deserialized before the  
Decls, so it will be trivial to pass it as a reference to Decl::Create.

Passing an ASTContext object also avoids the problem of having to do a  
typedef.  The real value of using an "Allocator" object is that one  
day we have the freedom to *not* use MallocAllocator.  Just passing  
the ASTContext object by reference and calling "getAllocator()" every  
time you allocate something removes the need for a typedef.  I also  
believe now that overall it is a much cleaner design to just pass the  
ASTContext object, as it basically encapsulates all the information we  
could ever want to pass down to the Type/Decl deserialization routines.

Ted