Fznamznon marked an inline comment as done.
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(); }
----------------
Fznamznon wrote:
> 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.
Ok, added a bit more test cases.
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
