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