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

[Manuel Klimek]

================
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");
----------------
.getPtr()

================
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");
----------------
.getPtr()

================
Comment at: lib/Tooling/Tooling.cpp:209
@@ -208,3 +251,2 @@
   Compiler.createSourceManager(*Files);
-  addFileMappingsTo(Compiler.getSourceManager());
 
----------------
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...

================
Comment at: include/clang/Tooling/Tooling.h:275
@@ -225,1 +274,3 @@
+  /// append them to ASTs.
+  int buildASTs(std::vector<ASTUnit *> &ASTs);
 
----------------
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...


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