[PATCH] D69934: [clangd] Implement rename by using SelectionTree and findExplicitReferences.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:326
+    // class Foo, but the token under the cursor is not corresponding to the
+    // "Foo" range, though the final result is correct.
     SourceLocation Loc = getBeginningOfIdentifier(
----------------
hokein wrote:
> ilya-biryukov wrote:
> > I would argue rename should not work in that case.
> > Could we check that the cursor is actually on the name token of the `NamedDecl` and not rename if it isn't?
> you are probably right, we only allow rename which is triggered on the name token. Will update the patch.
Definitely think we should do it before landing the patch.
Otherwise we'll introduce regressions.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+      tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
+  llvm::DenseSet<SymbolID> TargetIDs;
+  for (auto &USR : RenameUSRs)
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Why `USRs`? Could we just check whether the `ND.getCanonicalDecl()` is there instead?
> checking `ND.getCanonicalDecl()` is not enough, thinking about virtual methods.
> 
> `tooling::getUSRsForDeclaration` does all the black-magic things here, it returns all rename-related decls.
Could you add a comment that we need this to handle virtual methods?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:185
+      for (const auto *Target : Ref.Targets) {
+        auto ID = getSymbolID(Target);
+        assert(ID);
----------------
Are we sure that USRs will always work here?
I would be protective here, surely there are unhandled cases.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:225
   tooling::Replacements FilteredChanges;
-  for (const tooling::SymbolOccurrence &Rename :
-       findOccurrencesWithinFile(AST, RenameDecl)) {
-    // Currently, we only support normal rename (one range) for C/C++.
-    // FIXME: support multiple-range rename for objective-c methods.
-    if (Rename.getNameRanges().size() > 1)
-      continue;
-    // We shouldn't have conflicting replacements. If there are conflicts, it
-    // means that we have bugs either in clangd or in Rename library, therefore
-    // we refuse to perform the rename.
+  for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+    // We shouldn't have conflicting replacements or replacements from different
----------------
This seems to assume all occurrences are outside macros.
Won't it break in presence of macros?
Do we have tests when the renamed token is:
- inside macro body
- inside macro arguments
?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69934/new/

https://reviews.llvm.org/D69934