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