[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 9:13 AM, Matthijs Kooijman wrote:

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

Having the deletion of the ASTConsumer in ParseAST sounds fine to me.   
One request: please make DeleteConsumer have a default value of true.

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

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