hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:466 auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location()); if (DeclsUnderCursor.empty()) ---------------- kadircet wrote: > hokein wrote: > > kadircet wrote: > > > hokein wrote: > > > > kadircet wrote: > > > > > `locateDeclAt` is already working on `NamedDecl`s but returning a set > > > > > of `Decl`s could you rather update that helper to return a set of > > > > > `NamedDecl`s instead? > > > > I think the main problem is that `NamedDecl->getCanonicalDecl()` > > > > returns a `Decl*`, which we need to do a `dyn_cast`. > > > ah right, but still it should be safe to perform just an `llvm:cast` > > > here, as a `NamedDecl` shouldn't have an `unnamed` decl as its canonical > > > declaration. > > > a NamedDecl shouldn't have an unnamed decl as its canonical declaration. > > yeah, this is true for most cases, but it is not safe, we have corn cases, > > see the comment in SymbolCollector about ObjCPropertyDecl. > I don't think that's relevant in here though, it is performing the cast on a > `Decl`. It is the `ASTNode.OrigD` that has been populated by `libIndex`. > So it doesn't need to be related with whatever nameddecl is being processed. > > Am I missing something? ah, you are right. I thought ASTNode.OrigD was the decl getting from `getCanonicalDecl`, but it turns out not. I also checked with the `Decl::getCanonicalDecl` default implementation, if the Decl subclass doesn't override this method, it just returns itself. so it is safe for `NamedDecl`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73450/new/ https://reviews.llvm.org/D73450 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
