[PATCH] D73450: [clangd] Add a symbol-name-based blacklist for rename.

[Haojian Wu via Phabricator via cfe-commits]

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