Fznamznon 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(); }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > 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
> > Thanks for the extra perspective! Yeah, I think that's what I've been
> > convincing myself of. Effectively:
> > ```
> > int overloaded(int);
> > float overloaded(float) noexcept(error()); // error: invalid declaration
> >
> > int main() {
> > (void)overloaded(1.0f); // #1
> > }
> > ```
> > We're always going to get the invalid declaration diagnostic. The question
> > is whether we want a diagnostic at #1 that says "can't call this overload"
> > or whether we want #1 to have no diagnostic (because it picked the `int`
> > overload). From a purely diagnostic perspective, I think I can see
> > arguments either way. But if we change it slightly:
> > ```
> > template <typename Ty>
> > constexpr int func() { return 0; }
> >
> > template <>
> > constexpr int func<int>() { return 1; }
> >
> > template <>
> > constexpr int func<float>() { return 2; }
> >
> > int overloaded(int);
> > float overloaded(float) noexcept(error()); // error: invalid declaration
> >
> > static_assert(func(overloaded(1.0f)) == 2); // #1
> > ```
> > we still get the invalid declaration diagnostic, but now we get a failing
> > static assertion because we picked the `int` overload rather than the
> > `float` overload. I think this is clearly worse behavior than if we issued
> > an additional diagnostic at #1 saying that the overload we picked (`float`)
> > was invalid.
> >
> > https://godbolt.org/z/34aGMY43n
> >
> > WDYT?
> Agreed. If i write an overload i presumably want the compiler to consider it,
> even if I botched it. And apparently, the issue is not that we don't do that
> but rather than we don't always do that. We should strive to be consistent.
>
> It's not critical because even in the `static_assert` case, the only thing a
> user can do is fix the declaration, at which point they would get the
> ambiguity diag or the correct best match correctly picked. But it's nicer to
> have more diags, when we can
I see, thank you both for the extensive explanation.
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
