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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

It's a bit unclear how we manage to still rename overrides. Is this handled by the USR-generating functions?
Could we factor out the part that collects `NamedDecl`s and use those instead of the USRs instead?



================
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(
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+      tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
+  llvm::DenseSet<SymbolID> TargetIDs;
+  for (auto &USR : RenameUSRs)
----------------
Why `USRs`? Could we just check whether the `ND.getCanonicalDecl()` is there instead?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:97
+        };
+        struct C : B {
+          void [[f^oo]]() override {}
----------------
Could we stop at `B`?
I don't think `C->D` cover any more code paths, they merely add some noise to the test, making it harder to read.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:109
+          A a;
+          a.[[f^oo]]();
+          B b;
----------------
NIT: alternatively, just do `A().foo()` to make the test smaller


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:131
+        public:
+          [[Foo]](int value = 0) : x(value) {}
+
----------------
Could you also check that destructors are renamed properly?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:131
+        public:
+          [[Foo]](int value = 0) : x(value) {}
+
----------------
ilya-biryukov wrote:
> Could you also check that destructors are renamed properly?
NIT: maybe remove the bodies of the functions that don't have references to `Foo`?
They seem to merely add noise, don't think they improve coverage.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:185
+        };
+        // FIXME: selection tree.
+        Qux::Qux() : [[Foo]]() {}
----------------
The FIXME is a bit unclear. Could you explain in more detail?


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:214
+      R"cpp(
+        #define moo [[foo]]
+        int [[fo^o]]() { return 42; }
----------------
I would argue we should fail in that case and not rename.
If we change the macro body, there's a chance other usages that we didn't want to update will also be updated.

However, if the current rename does that, also happy to keep as is. Up to you.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:380
+
+    for (const auto &RenamePos : Code.points()) {
+      auto RenameResult =
----------------
NIT: also mention in the documentation comment above that rename is run on **all** points.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69934