sammccall added a comment.
Want to probe a bit at the behavior we're aiming for here.
TL;DR is I think the handling is basically right but considering the general
case tells us how to simplify filterBaseUsingDecl.
---
The testcase is great to illustrate the common case.
The thing we need to generalize it is multiple symbols (e.g. overloads).
namespace ns { int foo(int); char foo(char); }
using ns::foo;
int x = foo(42);
char y = foo('x');
What should happen when we rename `foo(int)` on line 1?
- rename both functions
- rename one function + the usingdecl
- rename one function and not the usingdecl
- rename one function and add a second usingdecl
- return an error
In general the UsingDecl will be in another file and not visible in the AST.
Index only knows "there's a reference there". So I think our only real option
is to rename one function + the UsingDecl.
(Assuming we want the common non-overloaded case to work. And assuming we don't
want to do something drastic like "silently rename overload sets together in
all cases").
What should happen when we rename `using ns::foo` on line 2?
- rename both functions
- rename one arbitrarily
- return an error
Renaming both functions sounds great here. However for now the rename
implementation requires a single canonical decl, so we need to return an error
for now.
If there's a single decl, renaming it seems fine.
What should happen when we rename `foo(42)` on line 3?
- rename both functions
- rename one function + the usingdecl
- rename one function and not the usingdecl
- rename one function and add a second usingdecl
- return an error
We *can* rely on the UsingDecl being visible here. However we should be
reasonably consistent with the first case, which I think rules out options 1, 3
and 5.
Splitting the usingdecl is a neat trick, but complicated, so let's start with
renaming one + functiondecl and maybe tack that on later.
================
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) {
----------------
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).
================
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:
> 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.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:832
+ )cpp",
};
llvm::StringRef NewName = "NewName";
----------------
Can you add a test case (or argue with me that the behavior should be something
else!):
```
namespace ns { int [[^foo]](int); char foo(char); }
using ns::[[foo]];
void f() {
[[^foo]](42);
foo('x');
}
```
And a negative case to Renameable:
```
namespace ns { int foo(int); char foo(char); }
using ns::^foo;
```
(the error should be "multiple symbols" ideally - "not a supported kind" is
confusing if we support renaming on a non-overload using)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135489/new/
https://reviews.llvm.org/D135489
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
