On Tue, 3 Dec 2024 at 01:58, Patrick Palka <ppa...@redhat.com> wrote:
>
> On Mon, 2 Dec 2024, Jonathan Wakely wrote:
>
> > On Mon, 2 Dec 2024 at 17:42, Patrick Palka <ppa...@redhat.com> wrote:
> > >
> > > On Mon, 2 Dec 2024, Patrick Palka wrote:
> > >
> > > > On Thu, 28 Nov 2024, Jonathan Wakely wrote:
> > > >
> > > > > As suggested by Jason, this makes all __normal_iterator operators into
> > > > > friends so they can be found by ADL and don't need to be separately
> > > > > exported in module std.
> > > >
> > > > Might as well remove the __gnu_cxx exports in std.cc.in while we're at
> > > > it?
> >
> > Ah yes, since that was the original purpose of the change!
> >
> > > > >
> > > > > For the operator<=> comparing two iterators of the same type, I had to
> > > > > use a deduced return type and add a requires-clause, because it's no
> > > > > longer a template and so we no longer get substitution failures when
> > > > > it's considered in oerload resolution.
> > > > >
> > > > > I also had to reorder the __attribute__((always_inline)) and
> > > > > [[nodiscard]] attributes, which have to be in a particular order when
> > > > > used on friend functions.
> > > > >
> > > > > libstdc++-v3/ChangeLog:
> > > > >
> > > > > * include/bits/stl_iterator.h (__normal_iterator): Make all
> > > > > non-member operators hidden friends.
> > > > > * src/c++11/string-inst.cc: Remove explicit instantiations of
> > > > > operators that are no longer templates.
> > > > > ---
> > > > >
> > > > > Tested x86_64-linux.
> > > > >
> > > > > This iterator type isn't defined in the standard, and users shouldn't
> > > > > be
> > > > > doing funny things with it, so nothing prevents us from replacing its
> > > > > operators with hidden friends.
> > > > >
> > > > > libstdc++-v3/include/bits/stl_iterator.h | 341
> > > > > ++++++++++++-----------
> > > > > libstdc++-v3/src/c++11/string-inst.cc | 11 -
> > > > > 2 files changed, 184 insertions(+), 168 deletions(-)
> > > > >
> > > > > diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> > > > > b/libstdc++-v3/include/bits/stl_iterator.h
> > > > > index e872598d7d8..656a47e5f76 100644
> > > > > --- a/libstdc++-v3/include/bits/stl_iterator.h
> > > > > +++ b/libstdc++-v3/include/bits/stl_iterator.h
[...]
> > > > > - template<typename _Iterator, typename _Container>
> > > > > - [[nodiscard, __gnu__::__always_inline__]]
> > > > > - constexpr bool
> > > > > - operator==(const __normal_iterator<_Iterator, _Container>& __lhs,
> > > > > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > > > > - noexcept(noexcept(__lhs.base() == __rhs.base()))
> > > > > - requires requires {
> > > > > - { __lhs.base() == __rhs.base() } -> std::convertible_to<bool>;
> > > > > - }
> > > > > - { return __lhs.base() == __rhs.base(); }
> > > > > + template<typename _Iter>
> > > > > + [[nodiscard, __gnu__::__always_inline__]]
> > > > > + friend
> > > > > + constexpr std::__detail::__synth3way_t<_Iterator, _Iter>
> > > > > + operator<=>(const __normal_iterator& __lhs,
> > > > > + const __normal_iterator<_Iter, _Container>& __rhs)
> > > > > + noexcept(__nothrow_synth3way<_Iter>)
> > > > > + requires requires {
> > > > > + std::__detail::__synth3way(__lhs.base(), __rhs.base());
> > > > > + }
> > > > > + { return std::__detail::__synth3way(__lhs.base(), __rhs.base()); }
> > > > >
> > > > > - template<typename _Iterator, typename _Container>
> > > > > - [[nodiscard, __gnu__::__always_inline__]]
> > > > > - constexpr std::__detail::__synth3way_t<_Iterator>
> > > > > - operator<=>(const __normal_iterator<_Iterator, _Container>&
> > > > > __lhs,
> > > > > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > > > > - noexcept(noexcept(std::__detail::__synth3way(__lhs.base(),
> > > > > __rhs.base())))
> > > > > - { return std::__detail::__synth3way(__lhs.base(), __rhs.base());
> > > > > }
> > > > > + [[nodiscard, __gnu__::__always_inline__]]
> > > > > + friend
> > > > > + constexpr bool
> > > > > + operator==(const __normal_iterator& __lhs, const
> > > > > __normal_iterator& __rhs)
> > > > > + noexcept(noexcept(__lhs.base() == __rhs.base()))
> > > > > + requires requires {
> > > > > + { __lhs.base() == __rhs.base() } -> std::convertible_to<bool>;
> > > > > + }
> > > > > + { return __lhs.base() == __rhs.base(); }
> > > > > +
> > > > > + [[nodiscard, __gnu__::__always_inline__]]
> > > > > + friend
> > > > > + constexpr auto
> > > > > + operator<=>(const __normal_iterator& __lhs,
> > > > > + const __normal_iterator& __rhs)
> > > > > + noexcept(__nothrow_synth3way<_Iterator>)
> > > > > + requires requires {
> > > > > + std::__detail::__synth3way(__lhs.base() == __rhs.base());
> > > >
> > > > Copy/paste typo here? s/==/,
> >
> > Fixed, thanks.
> >
> > > That this typo didn't cause a testsuite failure maybe suggests
> > > this more specialized overload is redundant? Could we remove it and
> > > rely only on the heterogenous operator<=> overload? (Then we could
> > > inline the __nothrow_synth3way helper.)
> >
> > It does seem to be redundant. I tried removing the homogeneous
> > operator== but that causes:
> >
> > FAIL: 24_iterators/normal_iterator/greedy_ops.cc -std=gnu++20 (test
> > for excess errors)
> >
> > That testcase is arguably unreasonable, but it's what we've been
> > testing and "supporting" for decades.
> >
> > If I make these changes to the testsuite then it fails without the
> > homogeneous operator<=>:
> >
> > --- a/libstdc++-v3/testsuite/24_iterators/normal_iterator/greedy_ops.cc
> > +++ b/libstdc++-v3/testsuite/24_iterators/normal_iterator/greedy_ops.cc
> > @@ -37,6 +37,9 @@ void test01()
> > iterator_type it(0);
> >
> > it == it;
> > +#ifdef __cpp_lib_three_way_comparison
> > + it <=> it;
> > +#endif
> > it != it;
> > it < it;
> > it <= it;
> > --- a/libstdc++-v3/testsuite/util/testsuite_greedy_ops.h
> > +++ b/libstdc++-v3/testsuite/util/testsuite_greedy_ops.h
> > @@ -28,6 +28,12 @@ namespace greedy_ops
> > X operator!=(T, T)
> > { return X(); }
> >
> > +#ifdef __cpp_lib_three_way_comparison
> > + template<typename T>
> > + X operator<=>(T, T)
> > + { return X(); }
> > +#endif
> > +
> > template<typename T>
> > X operator<(T, T)
> > { return X(); }
> >
> > But I think it's OK to say that nobody should write that nonsense in
> > C++20, because they can use constraints to write sensible operators
> > that don't break things.
>
> I wouldn't be against that. We'd effectively be partially reverting
> r12-5882-g2c7fb16b5283cf which reintroduced homogenous operator==/<=>
> for move_iterator and reverse_iterator as well.
>
> If we decide to keep the homogenous operator<=> for these iterator
> adaptors then we should define one for counted_iterator as well
> since it currently only has heterogenous operator<=>/== and so is
> susceptible to this ambiguity issue it seems.
I went back to this patch (from December) to try removing the
"redundant" operator<=> overloads we talked about.
For __normal_iterator and counted_iterator we don't need the
homogeneous operator<=> to avoid ambiguities with
testsuite_greedy_op.h (unless I modify the tests to add a
greedy_ops::operator<=> which we decided is not a reasonable thing to
define in C++20). It looks like r12-5882-g2c7fb16b5283cf didn't need
to add operator<=> for __normal_iterator (but I think the extra
operator== was needed).
For move_iterator and reverse_iterator removing those homogeneous
operator<=> overloads causes failures for the greedy_ops.cc tests.
That's because move_iterator and reverse_iterator have a full set of
operator< and operator<= and operator> and operator>= as well as
operator<=>, as required by the standard. Those are all heterogeneous,
but to avoid ambiguities with e.g. greedy_ops::operator<(T,T) we
provide a single homogeneous operator<=> which the compiler uses to
synthesize homogeneous operator< etc. and they are better matches than
greedy_ops::operator<
Without the extra operator<=> overloads I see:
FAIL: 24_iterators/move_iterator/greedy_ops.cc -std=gnu++20 (test for
excess errors)
FAIL: 24_iterators/reverse_iterator/greedy_ops.cc -std=gnu++20 (test
for excess errors)
So unless we want to stop supporting the greedy_ops tests in C++20 and
later, we either need to keep the non-standard same-type overloads of
operator<=> or spam out same-type overloads of operator< etc.
