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