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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


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


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:83
+  Expected<decltype(std::declval<OptionType>().getValue())>
+  evaluate(RefactoringRuleContext &) const {
+    return Opt->getValue();
----------------
ioeric wrote:
> This could use some comment, e.g. what is `getValue` expected to do?
I've simplified this by using a typealias in the option itself.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:88
+private:
+  std::shared_ptr<OptionType> Opt;
+};
----------------
ioeric wrote:
> Please explain why `shared_ptr` is needed here.
The same option can be used by multiple rules, hence options can be owned by multiple requirements in  different rules at once. I documented this.


================
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>
----------------
ioeric wrote:
> Please elaborate on what's valid or invalid.
Oops, copy-pasted comment. I fixed it up.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:74
+  struct OptionGatherer {
+    std::vector<std::shared_ptr<RefactoringOption>> &Options;
+
----------------
ioeric wrote:
> Why `shared_ptr`? Would `unique_ptr` work?
I refactored this to change the redundant ownership here.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:372
       if (Rule->hasSelectionRequirement()) {
         HasSelection = true;
+        if (!Subcommand.getSelection()) {
----------------
ioeric wrote:
> Wondering if it is sensible to make `selection` also a general option so that we don't need to special-case selections.
I think right now it's tricky because of the testing support. However, I think that it would be better to use a similar mechanism for selection option as well. I'll think about how we can reconcile the two approaches and will hopefully come up with a patch for it.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856