tom-anders marked 4 inline comments as done.
tom-anders added a comment.
In D135489#3844426 <https://reviews.llvm.org/D135489#3844426>, @sammccall wrote:
> 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").
Agreed.
> 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.
Agreed, I added a FIXME in the test that we might want to extend
canonicalRenameDecl to return multiple Decls (Probably a `llvm::DenseSet`?)
There already is a similar issue when renaming virtual methods with
`size_overridden_methods() > 1`.
> 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.
Sounds good, splitting the using decl would be fancy though, maybe also add a
FIXME for this?
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
