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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Sorry for the delay (most of us are OOO this week).



================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:48
+
+  virtual void visitRefactoringOptions(RefactoringOptionConsumer &Consumer) = 0;
 };
----------------
Please document the behavior here. What options are visited? How is the consumer used?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:62
+
+  /// Returns the set of refactoring options that a used when evaluating this
+  /// requirement.
----------------
`that a used`?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:78
+  std::vector<std::shared_ptr<RefactoringOption>>
+  getRefactoringOptions() const final override {
+    return {Opt};
----------------
Why return a vector instead of a single value if there is only one element?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:83
+  Expected<decltype(std::declval<OptionType>().getValue())>
+  evaluate(RefactoringRuleContext &) const {
+    return Opt->getValue();
----------------
This could use some comment, e.g. what is `getValue` expected to do?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:88
+private:
+  std::shared_ptr<OptionType> Opt;
+};
----------------
Please explain why `shared_ptr` is needed here.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:56
   auto Err = findError(std::get<Is>(Values)...);
-  if (Err)
+  if (Err) {
     return Consumer.handleError(std::move(Err));
----------------
nit: no braces around one liner in LLVM style.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:68
+/// Scans the tuple and returns a valid \c Error if any of the values are
+/// invalid.
+template <typename FirstT, typename... RestT>
----------------
Please elaborate on what's valid or invalid.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:74
+  struct OptionGatherer {
+    std::vector<std::shared_ptr<RefactoringOption>> &Options;
+
----------------
Why `shared_ptr`? Would `unique_ptr` work?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOption.h:42
+
+  virtual void passToConsumer(RefactoringOptionConsumer &Consumer) = 0;
+};
----------------
This could also use some comment. E.g. what is passed/cosumed? 


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOption.h:47
+template <typename OptionType>
+std::shared_ptr<OptionType> createRefactoringOption() {
+  static_assert(std::is_base_of<RefactoringOption, OptionType>::value,
----------------
Again, why `shared_ptr` instead of `unique_ptr`?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOptionConsumer.h:26
+/// declaration in this interface.
+class RefactoringOptionConsumer {
+public:
----------------
IIUC, you are using visitor pattern to "handle" options. If so, consider `s/consumer/visitor` and `s/handle/visit`, just to be clearer.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:372
       if (Rule->hasSelectionRequirement()) {
         HasSelection = true;
+        if (!Subcommand.getSelection()) {
----------------
Wondering if it is sensible to make `selection` also a general option so that we don't need to special-case selections.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856