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

[Manuel Klimek]

================
Comment at: include/clang/Tooling/Tooling.h:79
@@ -63,1 +78,3 @@
 
+  bool runInvocation(clang::CompilerInvocation *Invocation,
+                     FileManager *Files);
----------------
Does doxygen default to having the base class' comment here?

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

================
Comment at: lib/Tooling/Tooling.cpp:118
@@ +117,3 @@
+  Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+  Args.push_back(FileName.data());
+  return Args;
----------------
.str()? I don't think .data() is always 0-terminated.

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

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

================
Comment at: lib/Tooling/Tooling.cpp:174
@@ +173,3 @@
+    : CommandLine(CommandLine.vec()),
+      Action(new SingleFrontendActionFactory(FAction)), OwnsAction(true),
+      Files(Files) {}
----------------
I'm usually opposed to Owns* patterns, but I don't see a better way here (for now) without changing all the callers...

================
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());
----------------
Did you delete this comment intentionally?

================
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) {
----------------
I'd use getPtr().

================
Comment at: lib/Tooling/Tooling.cpp:209
@@ -208,3 +251,2 @@
   Compiler.createSourceManager(*Files);
-  addFileMappingsTo(Compiler.getSourceManager());
 
----------------
Huh? Why can we delete this?


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