[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review!



================
Comment at: include/clang/Tooling/CommonOptionsParser.h:116
+  bool HasError;
+  std::string ErrorMessage;
   std::unique_ptr<CompilationDatabase> Compilations;
----------------
arphaman wrote:
> Would it be better to have an `llvm::Error' instead of the two fields?
It would make the code a bit more complicated... we would need to convert between `Error` and strings a few times here and in the user code.


================
Comment at: include/clang/Tooling/Execution.h:76
+private:
+  ToolResults *Results;
+};
----------------
arphaman wrote:
> Why not `unique_ptr`/`shared_ptr`? Who owns the results?
The context creator owns the results. Here we only store a reference.


================
Comment at: include/clang/Tooling/Execution.h:136
+/// TUs in sequence.
+class StandaloneToolExecutor : public ToolExecutor {
+public:
----------------
arphaman wrote:
> Maybe this class and `InMemoryToolResults` should be in a separate header?
Moved `StandaloneToolExecutor` into a separate header. I am keeping `InMemoryToolResults` here since it might be used by other executors.


https://reviews.llvm.org/D34272