[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 Daniel,

> My justification to Ted for the at least having the Release() style interface to 
> the CodeGen interface is that it probably doesn't make sense to pass in a
> module that isn't empty (since if it has things, and they matter, it can easily
> break). 
Yeah, I agree. Hadn't thought of the not-empty point yet, but it really makes
sense.

> My preference would be to keep the new style of CodeGen interface and
> fix the problem upstream. 
Agreed.

> Having ParseAST not delete the Consumer may help, but it doesn't really solve
> the problem if you suddenly want to layer more Consumers on top of each other.
> In that case, the simplest thing is to just provide a Consumer which will stuff
> the CodeGen Module* result somewhere for access later. I agree that this isn't
> particularly aesthetically appealing however.
I'm not sure I follow you here? Why would you want to layer arbitrary
Consumers on top of each other? You could layer another Consumer over a
CodeGenerator, but having ParseAST not delete the Consumer shouldn't change
anything about that (especially if we make the deletion optional).

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.

Like this, adding a deleteConsumer argument to ParseAST sounds a lot more
reasonable as well?

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parseast.diff
Type: text/x-diff
Size: 2141 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20080807/f2515311/attachment.diff>
-------------- 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/f2515311/attachment.sig>