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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:143
+    }
+    auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
+    return tooling::createRenameAtomicChanges(
----------------
sammccall wrote:
> if this is a local refactor, and there are no USRs (symbol doesn't exist), is this an error that needs to be signaled somehow?
This case should not be happened IMO. If we find the `ND`, we will rename `ND` at least.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:154
+
+class LocalQualifiedRename final : public RefactoringAction {
+public:
----------------
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.
 


https://reviews.llvm.org/D39332