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

[Peter Collingbourne]

================
Comment at: include/clang/Tooling/Tooling.h:79
@@ -63,1 +78,3 @@
 
+  bool runInvocation(clang::CompilerInvocation *Invocation,
+                     FileManager *Files);
----------------
Manuel Klimek wrote:
> Does doxygen default to having the base class' comment here?
It does.  I've added a more informative comment.

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

================
Comment at: lib/Tooling/Tooling.cpp:118
@@ +117,3 @@
+  Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+  Args.push_back(FileName.data());
+  return Args;
----------------
Manuel Klimek wrote:
> .str()? I don't think .data() is always 0-terminated.
Yeah.  I didn't look too closely at the code I was factoring out but of course StringRef-formed-from-toNullTerminatedStringRef has different invariants from plain StringRef.  Fixed.

================
Comment at: lib/Tooling/Tooling.cpp:159
@@ +158,3 @@
+public:
+  SingleFrontendActionFactory(FrontendAction *FAction) : FAction(FAction) {}
+
----------------
Manuel Klimek wrote:
> Can we %s/FAction/Action/? I don't think the F gives me anything.
Done.

================
Comment at: lib/Tooling/Tooling.cpp:198-200
@@ -197,5 +243,1 @@
 
-  // ToolAction can have lifetime requirements for Compiler or its members, and
-  // we need to ensure it's deleted earlier than Compiler. So we pass it to an
-  // OwningPtr declared after the Compiler variable.
-  OwningPtr<FrontendAction> ScopedToolAction(ToolAction.take());
----------------
Manuel Klimek wrote:
> Did you delete this comment intentionally?
I decided to remove it because I thought it only made sense in the context of the former ToolInvocation which had ToolAction as a member.  But upon a rereading I think it makes sense to bring it back. Done.

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

================
Comment at: lib/Tooling/Tooling.cpp:130
@@ -119,1 +129,3 @@
+  ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef), ToolAction,
+                            &*Files);
 
----------------
Manuel Klimek wrote:
> I'd use getPtr().
Done.

================
Comment at: lib/Tooling/Tooling.cpp:339
@@ -311,3 +338,3 @@
     });
-    ToolInvocation Invocation(CommandLine, ActionFactory->create(), &Files);
+    ToolInvocation Invocation(CommandLine, Action, &*Files);
     for (int I = 0, E = MappedFileContents.size(); I != E; ++I) {
----------------
Manuel Klimek wrote:
> I'd use getPtr().
Done.


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