ioeric added inline comments.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:154
+
+class LocalQualifiedRename final : public RefactoringAction {
+public:
----------------
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.
https://reviews.llvm.org/D39332
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
