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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits