[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:109
+
+  const std::string &getErrorMessage() const { return ErrorMessage; }
+
----------------
hokein wrote:
> return `llvm::StringRef`?
I'm not a big fan of `StringRef` as a return value. Callers can simply use `StringRef s = parser.getErrorMessage()`, if they want to; otherwise, they would need to worry about the live time of `s` in `auto s = parser.getErrorMessage()`, if the interface returns a `StringRef`.


================
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;
----------------
hokein wrote:
> 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.
> 
> 
It would make the the interface a bit cumbersome to use - callers would need to make pairs all the time. And converting to pairs is an unnecessary performance overhead IMO. The consistency with `AllKVResults` is not that important after all.

We don't have a lot usages of `std::pair<std::string, std::string>` yet. I'll add an alias when needed. Currently, I think it makes sense to keep the result type explicit.


================
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
----------------
hokein wrote:
> nit: `all`
I think for-each is a more canonical name? E.g. we have `for_each` loop in C++.


================
Comment at: lib/Tooling/Execution.cpp:61
+    if (!Executor) {
+      OS << "Failed to create '" << I->getName()
+         << "': " << llvm::toString(Executor.takeError()) << "\n";
----------------
hokein wrote:
> 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? 
Yes, we simply return the first plugin that matches. 

It's hard to ensure that all executors have mutually exclusive arguments when more executors are added; it should be up to the executors to define unique (or as-unique-as-possible) arguments. For example, a map-reduce-based executor could require a "--mr" option to be set. The factory should simply return the first one that matches.

The point of this helper function is to hide the dispatching of different executors, so I don't think requiring users to specify the executor name is sensible. For example, users should only care about whether to set `--mr` instead of having to know the existence of different executors.


================
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"));
----------------
hokein wrote:
> any reason to make it static?
Added a comment explaining this. 


================
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 =
----------------
hokein wrote:
> nit: size_t, the same below.
`createExecutorFromCommandLineArgs` and, more importantly, the underlying library functions take `int`, so... :(


https://reviews.llvm.org/D34272