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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to