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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
