[cfe-commits] r54364 - in /cfe/trunk: Driver/ASTConsumers.cpp Driver/ASTConsumers.h Driver/clang.cpp include/clang/CodeGen/ModuleBuilder.h lib/CodeGen/CodeGenModule.cpp lib/CodeGen/CodeGenModule.h lib/CodeGen/ModuleBuilder.cpp

[Daniel Dunbar]

I don't particularly care for the conditional deletion an argument. Just from
a semantic view it complicates ownership and makes the code less obviously
correct on a quick visual inspection.

I think we should just pull the deletion out of ParseAST(). Its almost simpler
for the caller to use an owning pointer anyway.

I like adding a Finalize(), even though it is redundant, just because it matches
nicely with Initialize() and the intent is obvious. Not to mention I can imagine
it becoming useful later. There conceivably could be situations where the same
ASTConsumer is passed to ParseAST multiple times. Having the pairing of
Initialize/Finalize wrapping all other calls means the consumer can separate
semantic actions from set-up and tear-down ones.


 - Daniel

p.s. By the way Matthijs, my point about chained consumers was that I can
envision providing "filter" or "transform" consumers (in the stream) sense that 
just modify the AST, for example renaming an element or slicing out part of the 
program. This means that if you want to call additional methods on an 
ASTConsumer deeper in the chain you have to keep that pointer around yourself 
(or add some other mechanism for accessing child consumers).


----- Original Message ----
From: Ted Kremenek <kremenek at apple.com>
To: Matthijs Kooijman <matthijs at stdin.nl>
Cc: Daniel Dunbar <daniel at zuster.org>; cfe-commits at cs.uiuc.edu
Sent: Thursday, August 7, 2008 11:10:23 AM
Subject: Re: [cfe-commits] r54364 - in /cfe/trunk: Driver/ASTConsumers.cpp Driver/ASTConsumers.h Driver/clang.cpp include/clang/CodeGen/ModuleBuilder.h lib/CodeGen/CodeGenModule.cpp lib/CodeGen/CodeGenModule.h lib/CodeGen/ModuleBuilder.cpp


On Aug 7, 2008, at 10:32 AM, Matthijs Kooijman wrote:

> Hi Ted,
>
>
>> Having the deletion of the ASTConsumer in ParseAST sounds fine to me.
> I suppose you mean, making it conditional?

Yeah, that's what I meant.

>> One request: please make DeleteConsumer have a default value of true.
> It already has, see the .h file.

Ah yes, I looked at the wrong line.

>>>
>> HandleTranslationUnit essentially is a Finalize method.  It's the  
>> last
>> action called to ASTConsumer when the entire TranslationUnit has been
>> parsed.  We can either rename it to Finalize, or provide a separate
>> Finalize method.  It seems redundant to have two methods though  
>> since they
>> would serve the same purpose.  If you want cleanliness, you could  
>> have
>> HandleTranslationUnit just call Finalize, just to get the conceptual
>> matching that you want (the performance impact of this one call  
>> would be
>> completely insignificant).
> I don't really care either way, but having an explicit Finalize  
> seems useful.
> Just documenting the fact that HandleTranslationUnit is the final  
> call to
> ASTConsumer would probably do as well (I know I can read that from  
> the ParseAST
> code, but having it defined more clearly prevents assumptions from  
> breaking in
> the future).

I agree.  The documentation should be enhanced.

> On difference between Finalize and the existing  
> HandleTranslationUnit is that
> Finalize wouldn't need any arguments (also to be more consistent with
> Initialize).

I can go either way on this.  HandleTranslationUnit is an action  
indicating the translation unit has been parsed, and thus it makes  
sense to do CodeGen at that point.  Finalize indicates there is no  
more work.  It just happens that HandleTranslationUnit implies the  
latter.  I'm fine with having two methods if that makes things  
conceptually cleaner.


>> Actually, it seems that we could do the following:
>>
>>     virtual llvm::Module* ReleaseModule() {
>>       return M.take();
>>     }
>>
>>    virtual void HandleTranslationUnit(TranslationUnit& TU) {
>>       if (Diags.hasErrorOccurred()) {
>>         M.reset();
>>         return;
>>       }
>>
>>      if (Builder)
>>        Builder->Release();
>>   };
>>
>> Calling "reset" on M will both (a) free the module and (b) set the  
>> pointer
>> to NULL.
>
> Yup, that seems quite right.

Sounds good.  Did you want to apply these combined changes or should I?