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

Reply via email to