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?
>
> >
> > 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?
>
> >
> > - 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/==/,
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.)
>
> > + }
> > + { 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.
>
> > + operator==(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() == __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline bool
> > - operator==(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() == __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator==(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() == __rhs.base(); }
> >
> > - 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(); }
> > + template<typename _Iter>
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator!=(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() != __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline bool
> > - operator!=(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() != __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator!=(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() != __rhs.base(); }
> >
> > - // Random access 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(); }
> > + // Random access iterator requirements
> > + template<typename _Iter>
> > + friend
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator<(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() < __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX20_CONSTEXPR
> > - inline bool
> > - operator<(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() < __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX20_CONSTEXPR
> > + inline bool
> > + operator<(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() < __rhs.base(); }
> >
> > - 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(); }
> > + template<typename _Iter>
> > + friend
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator>(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() > __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline bool
> > - operator>(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() > __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator>(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() > __rhs.base(); }
> >
> > - 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(); }
> > + template<typename _Iter>
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator<=(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() <= __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline bool
> > - operator<=(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() <= __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator<=(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() <= __rhs.base(); }
> >
> > - 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(); }
> > + template<typename _Iter>
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator>=(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() >= __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline bool
> > - operator>=(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() >= __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline bool
> > + operator>=(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() >= __rhs.base(); }
> > #endif // three-way comparison
> >
> > - // _GLIBCXX_RESOLVE_LIB_DEFECTS
> > - // According to the resolution of DR179 not only the various comparison
> > - // operators but also operator- must accept mixed iterator/const_iterator
> > - // parameters.
> > - template<typename _IteratorL, typename _IteratorR, typename _Container>
> > + // _GLIBCXX_RESOLVE_LIB_DEFECTS
> > + // 179. Comparison of const_iterators to iterators doesn't work
> > + // According to the resolution of DR179 not only the various
> > comparison
> > + // operators but also operator- must accept mixed
> > iterator/const_iterator
> > + // parameters.
> > + template<typename _Iter>
> > #if __cplusplus >= 201103L
> > - // DR 685.
> > - [[__nodiscard__, __gnu__::__always_inline__]]
> > - constexpr auto
> > - operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
> > - const __normal_iterator<_IteratorR, _Container>& __rhs) noexcept
> > - -> decltype(__lhs.base() - __rhs.base())
> > + [[__nodiscard__, __gnu__::__always_inline__]]
> > + friend
> > + // _GLIBCXX_RESOLVE_LIB_DEFECTS
> > + // 685. reverse_iterator/move_iterator difference has invalid signatures
> > + constexpr auto
> > + operator-(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs) noexcept
> > + -> decltype(__lhs.base() - __rhs.base())
> > #else
> > - inline typename __normal_iterator<_IteratorL,
> > _Container>::difference_type
> > - operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
> > - const __normal_iterator<_IteratorR, _Container>& __rhs)
> > + friend
> > + difference_type
> > + operator-(const __normal_iterator& __lhs,
> > + const __normal_iterator<_Iter, _Container>& __rhs)
> > #endif
> > - { return __lhs.base() - __rhs.base(); }
> > + { return __lhs.base() - __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline typename __normal_iterator<_Iterator,
> > _Container>::difference_type
> > - operator-(const __normal_iterator<_Iterator, _Container>& __lhs,
> > - const __normal_iterator<_Iterator, _Container>& __rhs)
> > - _GLIBCXX_NOEXCEPT
> > - { return __lhs.base() - __rhs.base(); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline difference_type
> > + operator-(const __normal_iterator& __lhs, const __normal_iterator&
> > __rhs)
> > + _GLIBCXX_NOEXCEPT
> > + { return __lhs.base() - __rhs.base(); }
> >
> > - template<typename _Iterator, typename _Container>
> > - _GLIBCXX_NODISCARD __attribute__((__always_inline__))
> > _GLIBCXX_CONSTEXPR
> > - inline __normal_iterator<_Iterator, _Container>
> > - operator+(typename __normal_iterator<_Iterator,
> > _Container>::difference_type
> > - __n, const __normal_iterator<_Iterator, _Container>& __i)
> > - _GLIBCXX_NOEXCEPT
> > - { return __normal_iterator<_Iterator, _Container>(__i.base() + __n); }
> > + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> > + friend
> > + _GLIBCXX_CONSTEXPR
> > + inline __normal_iterator
> > + operator+(difference_type __n, const __normal_iterator& __i)
> > + _GLIBCXX_NOEXCEPT
> > + { return __normal_iterator(__i.base() + __n); }
> > + };
> >
> > _GLIBCXX_END_NAMESPACE_VERSION
> > } // namespace __gnu_cxx
> > diff --git a/libstdc++-v3/src/c++11/string-inst.cc
> > b/libstdc++-v3/src/c++11/string-inst.cc
> > index ec5ba41ebcd..b8806304949 100644
> > --- a/libstdc++-v3/src/c++11/string-inst.cc
> > +++ b/libstdc++-v3/src/c++11/string-inst.cc
> > @@ -110,14 +110,3 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >
> > _GLIBCXX_END_NAMESPACE_VERSION
> > } // namespace
> > -
> > -namespace __gnu_cxx _GLIBCXX_VISIBILITY(default)
> > -{
> > -_GLIBCXX_BEGIN_NAMESPACE_VERSION
> > -
> > - using std::S;
> > - template bool operator==(const S::iterator&, const S::iterator&);
> > - template bool operator==(const S::const_iterator&, const
> > S::const_iterator&);
> > -
> > -_GLIBCXX_END_NAMESPACE_VERSION
> > -} // namespace
> > --
> > 2.47.0
> >
> >
>
