[PATCH] D36075: [refactor] Initial support for refactoring action rules

[Eric Liu via Phabricator via cfe-commits]

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