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

[Manuel Klimek via Phabricator via cfe-commits]

klimek 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.
----------------
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?


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


================
Comment at: include/clang/Tooling/Execution.h:49
+public:
+  virtual ~ToolResults() {}
+  virtual void addResult(StringRef Key, StringRef Value) = 0;
----------------
I think = default is the new cool way.


================
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;
----------------
Why do we need to get the results via the interface? For example, in a map/reduce style setup getting the results is infeasible.


================
Comment at: include/clang/Tooling/Execution.h:76
+
+  virtual ToolResults *getToolResults() const { return Results; }
+
----------------
I think it's weird that this is virtual, but we also  have a default implementation that returns something that we require in the constructor. Either make that virtual, and don't put in a default implementation, or make it non-virtual.


================
Comment at: include/clang/Tooling/Execution.h:90
+private:
+  // A reference to the results container. Not owned!
+  ToolResults *Results;
----------------
I don't think we need to annotate every unowned pointer. Pointers are unowned by default, otherwise we'd use unique_ptr.


================
Comment at: include/clang/Tooling/Execution.h:164-166
+createExecutorFromCommandLineArgs(int &argc, const char **argv,
+                                  llvm::cl::OptionCategory &Category,
+                                  const char *Overview = nullptr);
----------------
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?


https://reviews.llvm.org/D34272