Re: [PATCH RFC(libstdc++)] c++: implement P2468R2, the equality operator you are looking for

[Jonathan Wakely <jwakely@redhat.com>]

On Mon, 7 Nov 2022 at 22:49, Jason Merrill <jason@redhat.com> wrote:
>
> On 11/7/22 12:04, Jonathan Wakely wrote:
> > On Mon, 7 Nov 2022 at 21:56, Jonathan Wakely <jwakely@redhat.com> wrote:
> >>
> >> On Mon, 7 Nov 2022 at 20:58, Jason Merrill <jason@redhat.com> wrote:
> >>>
> >>> Tested x86_64-pc-linux-gnu.  Jonathan, what do you want to do about the library
> >>> test failure?
> >>>
> >>> -- >8 --
> >>>
> >>> This paper is resolving the problem of well-formed C++17 code becoming
> >>> ambiguous in C++20 due to asymmetrical operator== being compared with itself
> >>> in reverse.  I had previously implemented a tiebreaker such that if the two
> >>> candidates were functions with the same parameter types, we would prefer the
> >>> non-reversed candidate.  But the committee went with a different approach:
> >>> if there's an operator!= with the same parameter types as the operator==,
> >>> don't consider the reversed form of the ==.
> >>>
> >>> So this patch implements that, and changes my old tiebreaker to give a
> >>> pedwarn if it is used.  I also noticed that we were giving duplicate errors
> >>> for some testcases, and fixed the tourney logic to avoid that.
> >>>
> >>> As a result, a lot of tests of the form
> >>>
> >>>    struct A { bool operator==(const A&); };
> >>>
> >>> need to be fixed to add a const function-cv-qualifier, e.g.
> >>>
> >>>    struct A { bool operator==(const A&) const; };
> >>>
> >>> The committee thought such code ought to be fixed, so breaking it was fine.
> >>>
> >>> 18_support/comparisons/algorithms/fallback.cc also breaks with this patch,
> >>> because of the similarly asymmetrical
> >>>
> >>>    bool operator==(const S&, S&) { return true; }
> >>>
> >>> I assume this was written this way deliberately, so I'm not sure what to do
> >>> about it.
> >>
> >> Yes, that was deliberate. The compare_strong_order_fallback function
> >> has these constraints:
> >>
> >> template<typename _Tp, __decayed_same_as<_Tp> _Up>
> >>    requires __strongly_ordered<_Tp, _Up> || __op_eq_lt<_Tp, _Up>
> >>    constexpr strong_ordering
> >>    operator() [[nodiscard]] (_Tp&& __e, _Up&& __f) const
> >>
> >> And similarly for the other fallbacks. So I wanted to check that two
> >> types that decay to the same type (like S and const S) can be compared
> >> with == and <, and therefore can be used with this function.
> >>
> >> But if such asymmetry is no longer allowed, maybe the library function
> >> is no longer usable with pathological cases like the test, and so the
> >> test should be changed. We can't just replace the decayed_same_as
> >> constraint with same_as because the std::strong_order customization
> >> point still supports similar types, but we could do:
> >>
> >> --- a/libstdc++-v3/libsupc++/compare
> >> +++ b/libstdc++-v3/libsupc++/compare
> >> @@ -1057,11 +1057,11 @@ namespace std _GLIBCXX_VISIBILITY(default)
> >>      };
> >>
> >>      template<typename _Tp, typename _Up>
> >> -      concept __op_eq_lt = requires(_Tp&& __t, _Up&& __u)
> >> +      concept __op_eq_lt = same_as<_Tp, _Up> && requires(_Tp&& __t)
> >>         {
> >> -         { static_cast<_Tp&&>(__t) == static_cast<_Up&&>(__u) }
> >> +         { static_cast<_Tp&&>(__t) == static_cast<_Tp&&>(__t) }
> >>             -> convertible_to<bool>;
> >> -         { static_cast<_Tp&&>(__t) < static_cast<_Up&&>(__u) }
> >> +         { static_cast<_Tp&&>(__t) < static_cast<_Tp&&>(__t) }
> >>             -> convertible_to<bool>;
> >>         };
> >
> > No wait, that's nonsense. We can still try to compare similar types,
> > it's just that they won't be comparable unless their comparison ops
> > have two parameters of the same type.
>
> Basically, though in this case the problem is that the arguments are the
> same type and the parameters are different.

Ah, so the operator== isn't actually rejected (despite being
pathologically dumb) it's just that some uses of it result in
ambiguities.

> >> And then adjust the test accordingly. If those fallbacks can no longer
> >> support mixed types, does the resolution of
> >> https://cplusplus.github.io/LWG/issue3465 even make sense now? If E
> >> and F must be the same type now, then E < F already implies F < E. I
> >> think we need some library changes to sync with P2468R2.
> >
> > I think this bit was right though. E and F might be different types,
> > but E < F implies F < E. Is that right?
>
> The P2468 changes only affect ==/!=, not <, so LWG3465 should be unaffected.

Gotcha.

> I think the only problem is the test itself: the <S,S> asserts need to
> be inverted because S and S cannot be compared for equality with the
> asymmetrical op== due to the normal candidate being better for arg 2 and
> the reversed candidate being better for arg 1.  The <const S, S> asserts
> are fine because the arguments match the asymmetry, so the normal
> candidate is better for both args.

Gotcha.

> So, the below fixes the test, does
> it make sense to you?

Yep, looks good. Thanks.