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