hokein added a comment.
don't dig into details, first round of comments.
I think the scope is a bit ambiguous, my reading is that it 1) removes old
clang-rename API usage *and* 2) fixes some existing bugs. I would suggest just
scoping it on 1).
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:251
+const NamedDecl *canonicalRenameDecl(const FieldDecl *D) {
+ // This is a hacky way to do something like
+ // CXXMethodDecl::getINstantiatedFromMemberFunction for the field because
----------------
yeah, this is quite tricky. I'd suggest to defer it to a separate patch.
The behavior you described in https://github.com/clangd/clangd/issues/582 makes
sense to me.
Another missing case is about static class members, they are `VarDecl` in the
AST.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:283
+ const auto *Definition = RD->getDefinition();
+ Candidate = Definition ? Definition : RD->getMostRecentDecl();
+ }
----------------
I'm not sure the motivation of the change, the same as line 244.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:299
+ if (const auto *Field = dyn_cast<FieldDecl>(Candidate))
+ return canonicalRenameDecl(Field);
+ return Candidate;
----------------
Looks like we're missing the `VarDecl` case (looks like a regression). I think
we should probably add tests from
https://github.com/llvm/llvm-project/commit/1e32df2f91f1aa1f8cd400ce50a621578fa0534e
to clangd rename test.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:300
+ return canonicalRenameDecl(Field);
+ return Candidate;
+}
----------------
we're comparing Decl pointers to check whether they point to the same symbol, I
think we should use `Candidate->getCanonicalDecl()` here.
thinking of a case, in the AST, we have two `Decl*`s, one points to the forward
decl, the other one points to the definition. but they points to the same
symbol.
```
class Foo; // forward decl, this is the canonical decl.
class Foo {
};
```
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:307
trace::Span Tracer("FindOccurrencesWithinFile");
- // If the cursor is at the underlying CXXRecordDecl of the
- // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
- // get the primary template manually.
- // getUSRsForDeclaration will find other related symbols, e.g. virtual and
its
- // overriddens, primary template and all explicit specializations.
- // FIXME: Get rid of the remaining tooling APIs.
- const auto *RenameDecl =
- ND.getDescribedTemplate() ? ND.getDescribedTemplate() : &ND;
- std::vector<std::string> RenameUSRs =
- tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
- llvm::DenseSet<SymbolID> TargetIDs;
- for (auto &USR : RenameUSRs)
- TargetIDs.insert(SymbolID(USR));
+ const auto *RenameDecl = canonicalRenameDecl(&ND);
----------------
since we call `canonicalRenameDecl` in `locateDeclAt`, I think `ND` is already
canonical, right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71880/new/
https://reviews.llvm.org/D71880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
