[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

[Matthijs Kooijman]

Hi Ted,


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

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

>> I've attached a small patch that makes the deletion conditional. This did 
>> turn
>> up a problem where the ASTContext was no longer valid (it was on the stack 
>> in
>> ParseAST) when calling Release(), so I moved the Builder->Release() call 
>> into
>> HandleTranslationUnit, so CodeGenerator::Release doesn't require any 
>> context
>> anymore.
>>
>> However, it seems slightly irky to use HandleTranslationUnit for this. 
>> Perhaps
>> it would be good to introduce a Finalize() method, which would be the 
>> converse
>> of the Initialize() method? Perhaps the LLVMCodeGenWriter could then also 
>> put
>> it's output writing in there, which is IMHO a lot cleaner than doing such
>> stuff in a destructor.
>
> 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).

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

> One comment on your patch.  The check:
>
>        if (Diags.hasErrorOccurred())
>
> should dominate:
>
>       if (Builder)
>         Builder->Release();
>
> in HandleTranslationUnit.  The reason for the check is that we don't want 
> to do LLVM IR generation if there are any parse errors.

Ah, I did think of this, but wanted to see if it would work at all first.
Forgot about it afterwards.

> 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.

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20080807/152d9f65/attachment.sig>