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