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
Index: lib/CodeGen/ModuleBuilder.cpp
===================================================================
--- lib/CodeGen/ModuleBuilder.cpp (revision 54461)
+++ lib/CodeGen/ModuleBuilder.cpp (working copy)
@@ -52,10 +52,7 @@
virtual llvm::Module* ReleaseModule() {
if (Diags.hasErrorOccurred())
return 0;
-
- if (Builder)
- Builder->Release();
-
+
return M.take();
}
@@ -134,6 +131,11 @@
virtual void HandleTagDeclDefinition(TagDecl *D) {
Builder->UpdateCompletedType(D);
}
+
+ virtual void HandleTranslationUnit(TranslationUnit& TU) {
+ if (Builder)
+ Builder->Release();
+ }
};
}
Index: lib/Sema/ParseAST.cpp
===================================================================
--- lib/Sema/ParseAST.cpp (revision 54461)
+++ lib/Sema/ParseAST.cpp (working copy)
@@ -27,7 +27,7 @@
/// ParseAST - Parse the entire file specified, notifying the ASTConsumer as
/// the file is parsed. This takes ownership of the ASTConsumer and
/// ultimately deletes it.
-void clang::ParseAST(Preprocessor &PP, ASTConsumer *Consumer, bool PrintStats) {
+void clang::ParseAST(Preprocessor &PP, ASTConsumer *Consumer, bool PrintStats, bool DeleteConsumer) {
// Collect global stats on Decls/Stmts (until we have a module streamer).
if (PrintStats) {
Decl::CollectingStats(true);
@@ -78,5 +78,6 @@
Stmt::CollectingStats(false);
}
- delete Consumer;
+ if (DeleteConsumer)
+ delete Consumer;
}
Index: include/clang/Sema/ParseAST.h
===================================================================
--- include/clang/Sema/ParseAST.h (revision 54461)
+++ include/clang/Sema/ParseAST.h (working copy)
@@ -21,7 +21,7 @@
/// ParseAST - Parse the entire file specified, notifying the ASTConsumer as
/// the file is parsed. This takes ownership of the ASTConsumer and
/// ultimately deletes it.
- void ParseAST(Preprocessor &pp, ASTConsumer *C, bool PrintStats = false);
+ void ParseAST(Preprocessor &pp, ASTConsumer *C, bool PrintStats = false, bool DeleteConsumer = true);
} // end namespace clang
#endif
signature.asc
Description: Digital signature
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
