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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D69934#1736729 <https://reviews.llvm.org/D69934#1736729>, @ilya-biryukov wrote:

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


Yes, it is handled by the tooling USR-generating function. I would not touch that part, I plan to get rid of that API  in a followup.



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


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+      tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
+  llvm::DenseSet<SymbolID> TargetIDs;
+  for (auto &USR : RenameUSRs)
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:97
+        };
+        struct C : B {
+          void [[f^oo]]() override {}
----------------
ilya-biryukov wrote:
> 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.
Stopped C.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:131
+        public:
+          [[Foo]](int value = 0) : x(value) {}
+
----------------
ilya-biryukov wrote:
> 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.
Done. Add a new test for constructor/destructor case.


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

This is a tricky case, if the cursor is on cxx initializer list, SelectionTree will return the CXXConsturctorExpr node of `Baz`, it actually rename the class `Baz` instead of renaming the `Foo` field.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:214
+      R"cpp(
+        #define moo [[foo]]
+        int [[fo^o]]() { return 42; }
----------------
ilya-biryukov wrote:
> 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.
I was also surprised with this testcase, but it aligns with what the current rename does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69934