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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Tooling/CommonOptionsParser.h:116
+  bool HasError;
+  std::string ErrorMessage;
   std::unique_ptr<CompilationDatabase> Compilations;
----------------
Would it be better to have an `llvm::Error' instead of the two fields?


================
Comment at: include/clang/Tooling/Execution.h:47
+  virtual ~ToolResults() {}
+  virtual void addResult(llvm::StringRef Key, llvm::StringRef Value) = 0;
+  virtual std::vector<std::pair<std::string, std::string>> AllKVResults() = 0;
----------------
You don't need to use the `llvm::` prefix for `StringRef`.


================
Comment at: include/clang/Tooling/Execution.h:76
+private:
+  ToolResults *Results;
+};
----------------
Why not `unique_ptr`/`shared_ptr`? Who owns the results?


================
Comment at: include/clang/Tooling/Execution.h:134
+
+/// \brief A stand alone executor that runs FrontendActions on a given set of
+/// TUs in sequence.
----------------
Standalone is one word.


================
Comment at: include/clang/Tooling/Execution.h:136
+/// TUs in sequence.
+class StandaloneToolExecutor : public ToolExecutor {
+public:
----------------
Maybe this class and `InMemoryToolResults` should be in a separate header?


https://reviews.llvm.org/D34272