[PATCH] Introduce ClangTool::buildASTs, and buildASTFromCode.

[Peter Collingbourne]

================
Comment at: include/clang/Tooling/Tooling.h:275
@@ -225,1 +274,3 @@
+  /// append them to ASTs.
+  int buildASTs(std::vector<ASTUnit *> &ASTs);
 
----------------
Manuel Klimek wrote:
> Peter Collingbourne wrote:
> > Manuel Klimek wrote:
> > > Hm. ClangTool is made to be extended, for example by RefactoringClangTool. I'm unsure how buildAST fits that interface. I've not read the rest of the patch yet, but it's unclear to me how buildAST here is related to run() from the comment. If it's not, why is it in ClangTool? If it is, I think we need to explain it here...
> > > Hm. ClangTool is made to be extended, for example by RefactoringClangTool. I'm unsure how buildAST fits that interface.
> > 
> > I think it may make sense for classes like RefactoringTool to derive from ClangTool as (I believe) RefactoringTool needs the ASTs it works with to use the same FileManager (i.e. it can't just use any old AST).  If that were not the case I would be arguing that the functionality in RefactoringTool should exist independently of ClangTool.
> > 
> > But I don't think it makes sense to allow run() to be overridden.  The purpose of run(), as I see it, is simple: it runs an action over a specified list of source files.  That is not necessarily the same thing as running a tool; a tool may need to do something before or after or instead of running the actions, but those things could just as easily be done by the caller. (In fact, I don't think this function should be called run() at all; that name implies that all Clang tools do all their work inside the run() function, which is not necessarily always true.  A better name may be something like runActions().)
> > 
> > It would make even less sense to me to allow buildASTs() to be overridden.  In the case where a refactoring tool uses buildASTs it will probably want to build the ASTs, do some work and then apply the replacements.  That strongly implies to me that the functionality for applying replacements should live in a separate member function in RefactoringTool.  (runAndSave() could still exist, but as a convenience function.)
> > 
> > > I've not read the rest of the patch yet, but it's unclear to me how buildAST here is related to run() from the comment. If it's not, why is it in ClangTool? If it is, I think we need to explain it here...
> > 
> > I see run and buildASTs as the two alternative ways to build a Clang tool: one callback-based and the other value-based. (buildASTs happens to be implemented using run, but that's not too important.) To that end, I think the tooling interface should have a buildASTs* function for each run* function.  If you agree, I'll update LibTooling.rst with this information.
> So,
> 1. after re-reading RefactoringTool I agree with you that run() shouldn't be virtual any more (in fact, I think we can just make it non-virtual now).
> 2. I still don't see the symmetry between run() (yes, runActions might be a better name) and buildAST - buildASTs doesn't run any actions, and it doesn't help me in running any actions; it seems completely unrelated to what the clang tool does (apart from that it might be implemented on top of buildASTs
> 
> It nearly seems like we're missing an abstraction between ToolInvocation and ClangTool... Hmmm...
> 2. I still don't see the symmetry between run() (yes, runActions might be a better name) and buildAST - buildASTs doesn't run any actions, and it doesn't help me in running any actions; it seems completely unrelated to what the clang tool does (apart from that it might be implemented on top of buildASTs

Likewise, a Clang tool may be implemented on top of runActions() -- running actions may only be a small part of what a given Clang tool does.  I see ClangTool more as a handy abstraction that lets you do a number of AST-related things with a set of source file paths -- exactly the kind of thing that a Clang tool may need to do.

> It nearly seems like we're missing an abstraction between ToolInvocation and ClangTool... Hmmm...

There could be a ToolInvocation-like thing that builds ASTs, but I think that's outside the scope of this patch.

================
Comment at: lib/Tooling/Tooling.cpp:209
@@ -208,3 +251,2 @@
   Compiler.createSourceManager(*Files);
-  addFileMappingsTo(Compiler.getSourceManager());
 
----------------
Manuel Klimek wrote:
> Peter Collingbourne wrote:
> > Manuel Klimek wrote:
> > > Huh? Why can we delete this?
> > The remappings are now created on the PreprocessorOptions on lines 213-219 instead of on the SourceManager directly.
> Hmmm... I remember doing this for a reason, but I don't remember the reason...
Well, the tests still pass. Could it be to do with ownership? As far as I can tell, SourceManager continues to take ownership of the MemoryBuffers (see the logic in InitializeFileRemapping in Frontend/InitPreprocessor.cpp).  In any case, ASTUnit clients need to provide the mappings in PreprocessorOptions.

================
Comment at: unittests/Tooling/ToolingTest.cpp:154
@@ -129,1 +153,3 @@
+  clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+                                            &*Files);
   Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
----------------
Manuel Klimek wrote:
> .getPtr()
Done.

================
Comment at: unittests/Tooling/ToolingTest.cpp:173
@@ -146,1 +172,3 @@
+  clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+                                            &*Files);
   Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
----------------
Manuel Klimek wrote:
> .getPtr()
Done.


http://llvm-reviews.chandlerc.com/D2097