[PATCH] D156244: [clang] Do not crash on use of a variadic overloaded operator
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 31 12:06:35 PDT 2023
aaron.ballman added subscribers: cor3ntin, hubert.reinterpretcast, rsmith.
aaron.ballman added inline comments.
================
Comment at: clang/test/SemaCXX/overloaded-operator-decl.cpp:64
+class E {};
+void operator+(E, ...) {} // expected-error{{overloaded 'operator+' cannot be variadic}}
+void d() { E() + E(); }
----------------
Fznamznon wrote:
> aaron.ballman wrote:
> > I think it might make sense to extend the test coverage for the other operators you can overload, just to demonstrate we diagnose them all consistently. WDYT?
> Okay, while trying to add more test cases I discovered that following
> ```
> class E {};
> bool operator<(const E& lhs, ...);
> auto operator<=>(const E& lhs, ...);
>
> void d() {
> E() < E();
> }
> ```
> crashes even with the patch since there is code searching for best overload candidate that doesn't consider possibility for them making variadic.
> The code around overloading is actually pretty inconsistent, somewhere invalid candidates are considered, and somewhere not, so I spent some time not knowing what to do.
> I'm now inclined that we just shouldn't consider invalid candidates like @shafik
> suggests. WDYY?
>
Overload resolution doesn't need to produce a candidate that's viable to call; C++ lets you resolve to the "best viable function" only to say "and that one wasn't good enough either." e.g., http://eel.is/c++draft/over#match.general-3
I've not yet spotted anything in http://eel.is/c++draft/over that says invalid declarations should/should not be added to the initial candidate set. I *think* the intention is that if name lookup can find the name, it goes into the candidate set. Then that set is processed to remove functions that are not viable (http://eel.is/c++draft/over.match.viable). Then we find the best viable function from that set.
I think we should be keeping the function in the candidate set so long as it matches the rules in http://eel.is/c++draft/over.match.viable even if the function is otherwise not viable. Otherwise, correcting an unrelated issue might change overload resolution to find a completely different function. e.g., in my example above, we'd select `void overloaded(int);` as the best viable function, but when the user corrects the `float` function, we'd change to call that instead. I think it's easier to understand what's going on when picking the `float` overload to begin with and saying "but we can't call that because it's busted".
CC @cor3ntin @hubert.reinterpretcast @rsmith for some extra opinions, as I'm not certain if I'm interpreting the standard correctly or not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156244/new/
https://reviews.llvm.org/D156244
More information about the cfe-commits
mailing list