[PATCH] D37856: [refactor] add support for refactoring options

[Eric Liu via Phabricator via cfe-commits]

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