[PATCH] D135489: [clangd] Fix rename for symbol introduced by UsingDecl

Fri Oct 7 19:33:08 PDT 2022

sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:169
+// For renaming, we're only interested in foo's declaration, so drop the other one
+void filterBaseUsingDecl(llvm::DenseSet<const NamedDecl *>& Decls) {
+  if (Decls.size() == 2) {
----------------
sammccall wrote:
> sammccall wrote:
> > tom-anders wrote:
> > > I'm not really happy with the name here, open for suggestions
> > This only comes up when renaming the UsingDecl itself (else we reach the UsingShadowDecl rather than this one).
> > 
> > I think we should just unconditionally drop the UsingDecl from the list. The target decls will be in the list, and we'll do the right thing (rename one if unambiguous, error if there are multiple).
> I'm not sure it's right to handle BaseUsingDecl instead of UsingDecl here.
> 
> The other case is UsingEnumDecl, and I don't see any reason to treat that as a non-renaming alias as opposed to a simple reference. It doesn't actually introduce an alias of the enum it names! (I see that we *are* treating it that way in FindTarget, but I guess we should just fix that instead).
> 
> Certainly if we *are* deliberately handling UsingEnumDecl here we should have a testcase for it.
I sent https://reviews.llvm.org/D135506 to remove this behavior from FindTarget.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135489/new/

https://reviews.llvm.org/D135489