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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

This is a good start, a few comments. Also I'd suggest running clang-format on this patch -- I saw a few places violate the code style.



================
Comment at: include/clang/Tooling/CommonOptionsParser.h:90
   ///
-  /// This constructor exits program in case of error.
+  /// If \p ExitOnError is set (default), This constructor exits program in case
+  /// of error; otherwise, this sets the error flag and stores error messages.
----------------
`... is set to true` for clarify.


================
Comment at: include/clang/Tooling/CommonOptionsParser.h:109
+
+  const std::string &getErrorMessage() const { return ErrorMessage; }
+
----------------
return `llvm::StringRef`?


================
Comment at: include/clang/Tooling/Execution.h:21
+//
+//  New executors can be registered as ToolExecutorPlugins via the
+//  `ToolExecutorPluginRegistry`. CLI tools can use
----------------
Consider moving this comment to ToolExecutor class to make it more discoverable?


================
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;
----------------
How about using `std::pair<std::string, std::string>` type here? It would make it consistent with the `allKVResults` below.

Also I think we can define an alias `KVResult` for this type.




================
Comment at: include/clang/Tooling/Execution.h:48
+    virtual void addResult(llvm::StringRef Key, llvm::StringRef Value) = 0;
+    virtual std::vector<std::pair<std::string, std::string>> AllKVResults() = 0;
+    virtual void
----------------
nit: `all`


================
Comment at: include/clang/Tooling/Execution.h:81
+  /// \brief Executes actions specified in the execution configuration.
+  virtual llvm::Error Execute(const ExecutionConfig &Config) = 0;
+
----------------
nit: `execute`


================
Comment at: lib/Tooling/Execution.cpp:61
+    if (!Executor) {
+      OS << "Failed to create '" << I->getName()
+         << "': " << llvm::toString(Executor.takeError()) << "\n";
----------------
Consider we have two plugins, the first iteration fails, and the second one succeeds, the code looks like still treating it as an error here? And if the case where there are more than one executor being created successfully, we just return the first one.

My understanding of the createExecutorFromCommandLineArgs's behavior is to find a proper `ToolExecutorPlugin` and create a `ToolExecutor` instance.

Maybe add a command-line flag like `--executor=<my_executor_name>` to make it straightforward to choose which registered executor is used, and make `StandaloneToolExecutor` as default? 


================
Comment at: lib/Tooling/Execution.cpp:73
+
+const char *StandaloneToolExecutor::ExecutorName = "StandaloneToolExector";
+
----------------
nit: `Executor`


================
Comment at: unittests/Tooling/ToolingTest.cpp:32
 
+using testing::ElementsAre;
+
----------------
Seems that it is unused.


================
Comment at: unittests/Tooling/ToolingTest.cpp:638
+         llvm::cl::OptionCategory &Category) override {
+    static llvm::cl::opt<bool> TestExecutor(
+        "test_executor", llvm::cl::desc("Use TestToolExecutor"));
----------------
any reason to make it static?


================
Comment at: unittests/Tooling/ToolingTest.cpp:667
+  std::vector<const char*> argv = {"prog", "--fake_flag_no_no_no", "f"};
+  int argc = argv.size();
+  auto Executor =
----------------
nit: size_t, the same below.


https://reviews.llvm.org/D34272