[PATCH] D156244: [clang] Do not crash on use of a variadic overloaded operator

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin 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(); }
----------------
aaron.ballman wrote:
> 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.
I'm not sure how much the standardese matter here as the declaration of the operators are ill-formed anyway.
But from a QOI perspective, i think keeping in the set everything the users might reasonably expect to be considered makes sense to me, as it *should* lead to better diagnostics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156244