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