[PATCH] D39332: [clang-refactor] Introduce "local-qualified-rename" action.

[Haojian Wu via Phabricator via cfe-commits]

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