ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Looks good with some nits.
================
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:73
+template <typename OptionType>
+class OptionRequirement : public RefactoringOptionsRequirement {
+public:
----------------
nit: `OptionRequirement` sounds more general than the base class
`RefactoringOptionsRequirement`.
================
Comment at: include/clang/Tooling/Refactoring/RefactoringOption.h:47
+ /// invoke the \c visit method with a reference to an std::string value.
+ virtual void passToVisitor(RefactoringOptionVisitor &Consumer) = 0;
+};
----------------
nit: s/Consumer/Visitor/
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:129
+ const cl::opt<std::string> &
+ getStringOption(const RefactoringOption &Opt) const {
+ auto It = StringOptions.find(&Opt);
----------------
We use templated method for adding options above but type-specific interfaces
(e.g. `getStringOption`) for getting options. I think we should make either
both templated or separate interfaces. I am a bit inclined to separate
interfaces for each type for clarity.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:149
+/// refactoring action rule.
+class CommandLineRefactoringOptionConsumer final
+ : public RefactoringOptionVisitor {
----------------
Visitor?
Repository:
rL LLVM
https://reviews.llvm.org/D37856
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
