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
> > > @@ -1164,188 +1164,215 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > const _Iterator&
> > > base() const _GLIBCXX_NOEXCEPT
> > > { return _M_current; }
> > > - };
> > >
> > > - // Note: In what follows, the left- and right-hand-side iterators are
> > > - // allowed to vary in types (conceptually in cv-qualification) so that
> > > - // comparison between cv-qualified and non-cv-qualified iterators be
> > > - // valid. However, the greedy and unfriendly operators in std::rel_ops
> > > - // will make overload resolution ambiguous (when in scope) if we don't
> > > - // provide overloads whose operands are of the same type. Can someone
> > > - // remind me what generic programming is about? -- Gaby
> > > + private:
> > > + // Note: In what follows, the left- and right-hand-side iterators
> > > are
> > > + // allowed to vary in types (conceptually in cv-qualification) so
> > > that
> > > + // comparison between cv-qualified and non-cv-qualified iterators
> > > be
> > > + // valid. However, the greedy and unfriendly operators in
> > > std::rel_ops
> > > + // will make overload resolution ambiguous (when in scope) if we
> > > don't
> > > + // provide overloads whose operands are of the same type. Can
> > > someone
> > > + // remind me what generic programming is about? -- Gaby
> > >
> > > #ifdef __cpp_lib_three_way_comparison
> > > - template<typename _IteratorL, typename _IteratorR, typename _Container>
> > > - [[nodiscard, __gnu__::__always_inline__]]
> > > - constexpr bool
> > > - operator==(const __normal_iterator<_IteratorL, _Container>& __lhs,
> > > - const __normal_iterator<_IteratorR, _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 bool
> > > + operator==(const __normal_iterator& __lhs,
> > > + const __normal_iterator<_Iter, _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 _IteratorL, typename _IteratorR, typename _Container>
> > > - [[nodiscard, __gnu__::__always_inline__]]
> > > - constexpr std::__detail::__synth3way_t<_IteratorR, _IteratorL>
> > > - operator<=>(const __normal_iterator<_IteratorL, _Container>& __lhs,
> > > - const __normal_iterator<_IteratorR, _Container>& __rhs)
> > > - noexcept(noexcept(std::__detail::__synth3way(__lhs.base(),
> > > __rhs.base())))
> > > - { return std::__detail::__synth3way(__lhs.base(), __rhs.base()); }
> > > + template<typename _Iter>
> > > + static constexpr bool __nothrow_synth3way
> > > + = noexcept(std::__detail::__synth3way(std::declval<_Iterator&>(),
> > > + std::declval<_Iter&>()));
> >
> > Since base() returns a const reference do we want to consider const
> > references in this noexcept helper?
Good catch.
> >
> > >
> > > - 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.
> > > + }
> > > + { return std::__detail::__synth3way(__lhs.base(), __rhs.base()); }
> > > #else
> > > - // Forward iterator requirements
> > > - template<typename _IteratorL, typename _IteratorR, typename _Container>
> > > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > > _GLIBCXX_CONSTEXPR
> > > - inline bool
> > > - operator==(const __normal_iterator<_IteratorL, _Container>& __lhs,
> > > - const __normal_iterator<_IteratorR, _Container>& __rhs)
> > > - _GLIBCXX_NOEXCEPT
> > > - { return __lhs.base() == __rhs.base(); }
> > > + // Forward iterator requirements
> > > + template<typename _Iter>
> > > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > > + friend
> > > + _GLIBCXX_CONSTEXPR
> > > + inline bool
> >
> > IIUC hidden friends are implicitly inline so we can remove the inline
> > keyword here.
Yes, done here and for the ones below, thanks.
