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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
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.
----------------
klimek wrote:
> hokein wrote:
> > `... is set to true` for clarify.
> If we want error handling, why not make it a static factory that returns an ErrorOr instead?
Done. I pulled the changes for `CommonOptionsParser` into a separate patch (D39042 ). PTAL


================
Comment at: include/clang/Tooling/Execution.h:26-27
+//
+//  This is still experimental but expected to replace the existing `ClangTool`
+//  interface.
+//
----------------
klimek wrote:
> I'd remove this line. Those tend to only get stale without anybody changing them :)
Right :)


================
Comment at: include/clang/Tooling/Execution.h:51-53
+  virtual std::vector<std::pair<std::string, std::string>> AllKVResults() = 0;
+  virtual void forEachResult(
+      llvm::function_ref<void(StringRef Key, StringRef Value)> Callback) = 0;
----------------
klimek wrote:
> Why do we need to get the results via the interface? For example, in a map/reduce style setup getting the results is infeasible.
This would enable tools to access results regardless of the underlying representation of the results.

In a map/reduce style execution, we could have an implementation that deals with fetching and reading remote files; information about remote files can be fed into the implementation from the executor that performs such execution. It might make sense to add an interface that returns the metadata about the underlying data though (e.g. URI for remote files). WDYT?


================
Comment at: include/clang/Tooling/Execution.h:164-166
+createExecutorFromCommandLineArgs(int &argc, const char **argv,
+                                  llvm::cl::OptionCategory &Category,
+                                  const char *Overview = nullptr);
----------------
klimek wrote:
> I think for clang-refactor we'll need one level of abstraction in the middle:
> We'll need to be able to say "run on all translation units referencing symbol X" - how will this fit into the design?
> Also, how will code changes generally fit with this design?
> I think for clang-refactor we'll need one level of abstraction in the middle:
> We'll need to be able to say "run on all translation units referencing symbol X" - how will this fit into the design?
Yes. Firstly, I think clang-refactor is responsible for getting all TUs that reference a symbols and feeding the set of TUs into the tool interface. And then we could implements a "smart" executor that picks an appropriate execution (e.g. delegate to standalone executor or map/reduce style execution) according to the number of TUs. 

> Also, how will code changes generally fit with this design?
Code changes will be reported by tools/actions via the `ToolResults` interface as key-value pairs (e.g. source location + serialized `AtomicChange`). After the execution, callers would have access to the results in `ToolResults`, or we could also provide an interface that returns the information about the results so that they can be accessed later.


https://reviews.llvm.org/D34272