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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
