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