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