ioeric added inline comments.
================ Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26 +template <typename T> +detail::SourceSelectionRequirement< + typename selection::detail::EvaluateSelectionChecker< ---------------- arphaman wrote: > ioeric wrote: > > Could you help me understand this class? > > > > This seems to be a selection-specific requirement and should live in > > `selection`. It is also used in `BaseSpecializedRule` which seems to be a > > higher level of abstraction. > It's just a container class that stores all information about the > `requiredSelection` requirement. I agree about `BaseSpecializedRule`, that > connection should be chopped. I will move the evaluation code into the > requirement itself when I update the patch. > > I would tend to disagree about moving it though, as > `SourceSelectionRequirement` is a requirement first and I think that's why it > should live with other requirements. Yes, it's related to selections, but it > uses them to implement the requirement. I think it's better to keep > requirements together, as opposed to having `option` requirements close to > options, selection requirements close to selection, and so on. WDYT? Thanks! Makes sense. We might want to put individual requirements into their own headers so that this doesn't grow into a huge file when more requirements are supported. ================ Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h:42 + RequirementBase>::type { + using OutputType = typename DropExpectedOptional<OutputT>::Type; + ---------------- It might worth having a comment explaining why and how `Expected<Optional>` is wrapped and unwrapped during the evaluation. ================ Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33 +/// +/// - requiredSelection: The refactoring function won't be invoked unless the +/// given selection requirement is satisfied. ---------------- We might want to document supported requirements somewhere else so that we don't need to update this file every time a new requirement is added. ================ Comment at: include/clang/Tooling/Refactoring/RefactoringOperation.h:1 +//===--- RefactoringOperationController.h - Clang refactoring library -----===// +// ---------------- s/RefactoringOperationController.h/RefactoringOperation.h/ :) ================ Comment at: include/clang/Tooling/Refactoring/RefactoringOperation.h:29 +/// to represent a selection in an editor. +class RefactoringOperation { +public: ---------------- I found the name a bit confusing - `RefactoringOperation` sounds a bit like `RefactoringAction`. Would it make sense to call this `RefactoringContext` or `RefactoringRuleContext`, if this stores states of a refactoring rule? ================ Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21 +struct RefactoringResult { + enum ResultKind { + /// A set of source replacements represented using a vector of ---------------- arphaman wrote: > ioeric wrote: > > I'm a bit unsure about the abstraction of the refactoring result. I would > > expected refactoring results to be source changes always. Do you have any > > refactoring tool that outputs occurrences in mind? > In Xcode we require rename to return symbol occurrences because the IDE is > responsible for figuring out: > 1) The new name. Thus we can't produce replacements when renaming because we > don't know what the new name is when gathering the occurrences. > 2) Whether these occurrences should be actually applied. Thus we can't > produce replacements because it's up to the user to decide if they want to > rename some occurrence in a comment for example. > > In general 2) can be applied to tools like clang-refactor that could allow > users to select occurrences that don't guarantee a direct semantic match > (comments, etc.) in an interactive manner. > > I think clangd has a rather naive rename support, so these points may not > apply, but we may want to extend clangd's support for rename in the future as > well. I feel occurrences are more of an intermediate state of a refactoring action than a result. I'm wondering if it makes sense to introduce a separate class to represent such intermediate states? I am a bit nervous to fuse multiple classes into one; the interfaces can get pretty ugly when more result kinds are added. ================ Comment at: lib/Tooling/Refactoring/SourceSelectionConstraints.cpp:13 + +using namespace clang; +using namespace tooling; ---------------- We are tempted to avoid `using namespace` if possible. Repository: rL LLVM https://reviews.llvm.org/D36075 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits