[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

[Ted Kremenek]

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?