Am Do., 12. Juni 2025 um 10:22 Uhr schrieb Jonathan Wakely <
jwak...@redhat.com>:
> On Thu, 12 Jun 2025 at 09:21, Jonathan Wakely <jwak...@redhat.com> wrote:
> >
> > On Thu, 12 Jun 2025 at 08:12, Daniel Krügler <daniel.krueg...@gmail.com>
> wrote:
> > >
> > > Am Do., 12. Juni 2025 um 00:10 Uhr schrieb Jonathan Wakely <
> jwak...@redhat.com>:
> > >>
> > >> 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.
> > >>
> > >> The operator<=> comparing two iterators of the same type is removed
> > >> entirely, instead of being made a hidden friend. That overload was
> added
> > >> by r12-5882-g2c7fb16b5283cf to deal with unconstrained operator
> > >> overloads found by ADL, as defined in the testsuite_greedy_ops.h
> header.
> > >> We don't actually test that case as there's no unconstrained <=> in
> that
> > >> header, and it doesn't seem reasonable for anybody to define such an
> > >> operator<=> in C++20 when they should constrain their overloads
> properly
> > >> (e.g. using a requires-clause). The heterogeneous operator<=>
> overloads
> > >> added for reverse_iterator and move_iterator could also be removed,
> but
> > >> that's not part of this commit.
> > >>
> > >> 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, except ...
> > >> (operator<=>(__normal_iterator<I,C>, __normal_iterator<I,C>)):
> > >> Remove.
> > >> * src/c++11/string-inst.cc: Remove explicit instantiations of
> > >> operators that are no longer templates.
> > >> * src/c++23/std.cc.in (__gnu_cxx): Do not export operators
> for
> > >> __normal_iterator.
> > >> ---
> > >>
> > >> v2: removed the unnecessary operator<=>, removed std.cc exports, fixed
> > >> other minor issues noticed by Patrick.
> > >>
> > >> Tested x86_64-linux.
> > >>
> > >> libstdc++-v3/include/bits/stl_iterator.h | 327
> ++++++++++++-----------
> > >> libstdc++-v3/src/c++11/string-inst.cc | 11 -
> > >> libstdc++-v3/src/c++23/std.cc.in | 9 -
> > >> 3 files changed, 169 insertions(+), 178 deletions(-)
> > >>
> > >> diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> b/libstdc++-v3/include/bits/stl_iterator.h
> > >> index 478a98fe8a4f..a7188f46f6db 100644
> > >> --- a/libstdc++-v3/include/bits/stl_iterator.h
> > >> +++ b/libstdc++-v3/include/bits/stl_iterator.h
> > >> @@ -1164,188 +1164,199 @@ _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());
> }
> > >> + [[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(); }
> > >>
> > >> - 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 _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());
> }
> > >> + 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(noexcept(std::__detail::__synth3way(__lhs.base(),
> __rhs.base())))
> > >> + requires requires {
> > >> + std::__detail::__synth3way(__lhs.base(), __rhs.base());
> > >> + }
> > >> + { 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
> > >
> > >
> > > Just curious: So in this case the attribute order intentionally
> potentially deviates from the order of the other friend functions using
> [[nodiscard, __gnu__::__always_inline__]]?
> >
> > Yes because operator== needs to compile as C++98 so it uses
> > __attribute__((always_inline)) not [[gnu::always_inline]], and Clang
> > will only allow __attribute__((foo)) [[bar]] and not the other way
> > around:
> > https://godbolt.org/z/nGWss9YG3
> >
> > <source>:5:36: error: an attribute list cannot appear here
> > 5 | __attribute__((always_inline)) [[nodiscard]]
> > | ^~~~~~~~~~~~~
>
> I'll update the commit msg to note that the order is needed for Clang,
> as GCC seems happy with either order.
>
> >
> > We could change [[nodiscard,gnu::always_inline]] to be in the opposite
> > order, but when either order works, I prefer to have the standard
> > attribute first and the non-standard one second.
>
That would be helpful - thanks,
- Daniel
