hokein marked an inline comment as done.
hokein added inline comments.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:154
+
+class LocalQualifiedRename final : public RefactoringAction {
+public:
----------------
ioeric wrote:
> arphaman wrote:
> > ioeric wrote:
> > > ioeric wrote:
> > > > arphaman wrote:
> > > > > hokein wrote:
> > > > > > sammccall wrote:
> > > > > > > As discussed offline, it's not clear why this is a separate
> > > > > > > Action, rather than a different Rule that's part of the same
> > > > > > > Action.
> > > > > > >
> > > > > > > @arphaman how does the framework answer this question?
> > > > > > There is a
> > > > > > [document](https://clang.llvm.org/docs/RefactoringEngine.html#refactoring-action-rules)
> > > > > > describing it, but still ambiguous.
> > > > > >
> > > > > > We also had some questions about `local-rename` from the
> > > > > > discussion, need @arphaman's input:
> > > > > >
> > > > > > * `OccurrenceFinder` is not exposed now, it is merely used in
> > > > > > `RenameOccurrences`. We think there should be a public interface to
> > > > > > the clients, like for implementing interactive mode in IDE?
> > > > > > * Currently the rules defined in the same action must have mutual
> > > > > > command-line options, otherwise clang-refactor would complain the
> > > > > > command-line option are being registered more than once. It might
> > > > > > be very strict for some cases. For example, `-new-name` is most
> > > > > > likely being used by many rules in `local-rename` action.
> > > > > >
> > > > > I think that this should be just a rule in `local-rename`.
> > > > >
> > > > > So you'd be able to call:
> > > > >
> > > > > `clang-refactor local-rename -selection=X -new-name=foo`
> > > > > `clang-refactor local-rename -old-qualified-name=bar -new-name=foo`.
> > > > We need your help to understand how exactly `local-rename` is intended
> > > > to be used.
> > > >
> > > > From the current code and previous conversations we had, it seems to me
> > > > that the action would support the use case where a user selects an
> > > > identifier in the editor (say, with cursor) and initiates a
> > > > `local-rename` action but without providing the new name in the
> > > > beginning. The rename rule finds and returns all occurrences (including
> > > > token ranges) to the editor, and users can then start typing in the
> > > > new name, and in the same time, the editor performs text replacements
> > > > according to ranges of occurrences and the new name typed in. Is this
> > > > how `RenameOccurrences` is intended to be used in the future?
> > > >
> > > > If this is how `local-rename` is expected to be used, it would be hard
> > > > to merge qualified rename into it, because both qualified old name and
> > > > new name are required in order to calculate the range of a symbol
> > > > reference, and this doesn't fit with the above workflow. But if my
> > > > understanding is simply wrong (e.g. the editor would invoke
> > > > `local-rename` again to perform the actual refactoring), then I think
> > > > it makes a lot of sense to merge qualified rename into the current
> > > > local-rename action.
> > > Sorry, by "your help", I was referring to Alex ;) @arphaman
> > You're right that rename should deal with occurrences conceptually, but I
> > believe that's more of requirement imposed onto the editor clients. Rename
> > in particular is basically impossible to map to all clients using just one
> > generic model, so I think it's fine if `RenameOccurrences` class returns
> > source replacements that `local-rename` in `clang-refactor` consumes. I
> > don't think this will change in the future, if anything we will lift
> > `OccurrenceFinder`class into the header so that the editor clients can use
> > it.
> > So I think in terms of the tool it should be ok to have immediate
> > `local-rename` action that behaves similarly to `clang-rename` and deals
> > with source changes and not replacements.
> Thanks for the clarification! In that case, I agree that qualified rename
> should be a rule in the local-rename action.
Thanks for the explanation, that makes sense. Moved to `local-rename` action.
https://reviews.llvm.org/D39332
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
