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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
