On Tue, 15 Oct 2024, Jonathan Wakely wrote:
> Tested x86_64-linux.
>
> -- >8 --
>
> The __gnu_cxx::__normal_iterator type we use for std::vector::iterator
> is not specified by the standard, it's an implementation detail. This
> means it's not constrained by the rule that forbids strengthening
> constexpr. We can make it meet the constexpr iterator requirements for
> older standards, not only when it's required to be for C++20.
>
> For the non-const member functions they can't be constexpr in C++11, so
> use _GLIBCXX14_CONSTEXPR for those. For all constructors, const members
> and non-member operator overloads, use _GLIBCXX_CONSTEXPR or just
> constexpr.
>
> We can also liberally add [[nodiscard]] and [[gnu::always_inline]]
> attributes to those functions.
>
> Also change some internal helpers for std::move_iterator which can be
> unconditionally constexpr and marked nodiscard.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/stl_iterator.h (__normal_iterator): Make all
> members and overloaded operators constexpr before C++20.
> (__niter_base, __niter_wrap, __to_address): Add nodiscard
> and always_inline attributes.
> (__make_move_if_noexcept_iterator, __miter_base): Add nodiscard
> and make unconditionally constexpr.
> ---
> libstdc++-v3/include/bits/stl_iterator.h | 125 ++++++++++++++---------
> 1 file changed, 76 insertions(+), 49 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> b/libstdc++-v3/include/bits/stl_iterator.h
> index 85b98ffff61..3cc10a160bd 100644
> --- a/libstdc++-v3/include/bits/stl_iterator.h
> +++ b/libstdc++-v3/include/bits/stl_iterator.h
> @@ -656,7 +656,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> template<typename _Iterator>
> _GLIBCXX20_CONSTEXPR
> - auto
> + inline auto
> __niter_base(reverse_iterator<_Iterator> __it)
> -> decltype(__make_reverse_iterator(__niter_base(__it.base())))
> { return __make_reverse_iterator(__niter_base(__it.base())); }
> @@ -668,7 +668,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> template<typename _Iterator>
> _GLIBCXX20_CONSTEXPR
> - auto
> + inline auto
These 'inline' additions aren't mentioned in the ChangeLog it seems. Is
the intent of these inlines solely as a compiler hint?
> __miter_base(reverse_iterator<_Iterator> __it)
> -> decltype(__make_reverse_iterator(__miter_base(__it.base())))
> { return __make_reverse_iterator(__miter_base(__it.base())); }
> @@ -1060,23 +1060,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> using iterator_concept = std::__detail::__iter_concept<_Iterator>;
> #endif
>
> - _GLIBCXX_CONSTEXPR __normal_iterator() _GLIBCXX_NOEXCEPT
> - : _M_current(_Iterator()) { }
> + __attribute__((__always_inline__))
> + _GLIBCXX_CONSTEXPR
> + __normal_iterator() _GLIBCXX_NOEXCEPT
> + : _M_current() { }
>
> - explicit _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + explicit _GLIBCXX_CONSTEXPR
> __normal_iterator(const _Iterator& __i) _GLIBCXX_NOEXCEPT
> : _M_current(__i) { }
>
> // Allow iterator to const_iterator conversion
> #if __cplusplus >= 201103L
> template<typename _Iter, typename = __convertible_from<_Iter>>
> - _GLIBCXX20_CONSTEXPR
> + [[__gnu__::__always_inline__]]
> + constexpr
> __normal_iterator(const __normal_iterator<_Iter, _Container>& __i)
> noexcept
> #else
> // N.B. _Container::pointer is not actually in container requirements,
> // but is present in std::vector and std::basic_string.
> template<typename _Iter>
> + __attribute__((__always_inline__))
> __normal_iterator(const __normal_iterator<_Iter,
> typename __enable_if<
> (std::__are_same<_Iter, typename _Container::pointer>::__value),
> @@ -1085,17 +1090,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> : _M_current(__i.base()) { }
>
> // Forward iterator requirements
> - _GLIBCXX20_CONSTEXPR
> +
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> reference
> operator*() const _GLIBCXX_NOEXCEPT
> { return *_M_current; }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> pointer
> operator->() const _GLIBCXX_NOEXCEPT
> { return _M_current; }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator&
> operator++() _GLIBCXX_NOEXCEPT
> {
> @@ -1103,13 +1112,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return *this;
> }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator
> operator++(int) _GLIBCXX_NOEXCEPT
> { return __normal_iterator(_M_current++); }
>
> // Bidirectional iterator requirements
> - _GLIBCXX20_CONSTEXPR
> +
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator&
> operator--() _GLIBCXX_NOEXCEPT
> {
> @@ -1117,38 +1129,46 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return *this;
> }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator
> operator--(int) _GLIBCXX_NOEXCEPT
> { return __normal_iterator(_M_current--); }
>
> // Random access iterator requirements
> - _GLIBCXX20_CONSTEXPR
> +
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> reference
> operator[](difference_type __n) const _GLIBCXX_NOEXCEPT
> { return _M_current[__n]; }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator&
> operator+=(difference_type __n) _GLIBCXX_NOEXCEPT
> { _M_current += __n; return *this; }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> __normal_iterator
> operator+(difference_type __n) const _GLIBCXX_NOEXCEPT
> { return __normal_iterator(_M_current + __n); }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__))
> + _GLIBCXX14_CONSTEXPR
> __normal_iterator&
> operator-=(difference_type __n) _GLIBCXX_NOEXCEPT
> { _M_current -= __n; return *this; }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> __normal_iterator
> operator-(difference_type __n) const _GLIBCXX_NOEXCEPT
> { return __normal_iterator(_M_current - __n); }
>
> - _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> + _GLIBCXX_CONSTEXPR
> const _Iterator&
> base() const _GLIBCXX_NOEXCEPT
> { return _M_current; }
> @@ -1164,7 +1184,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> #if __cpp_lib_three_way_comparison
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - [[nodiscard]]
> + [[nodiscard, __gnu__::__always_inline__]]
> constexpr bool
> operator==(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1175,7 +1195,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() == __rhs.base(); }
>
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - [[nodiscard]]
> + [[nodiscard, __gnu__::__always_inline__]]
> constexpr std::__detail::__synth3way_t<_IteratorR, _IteratorL>
> operator<=>(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1183,7 +1203,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return std::__detail::__synth3way(__lhs.base(), __rhs.base()); }
>
> template<typename _Iterator, typename _Container>
> - [[nodiscard]]
> + [[nodiscard, __gnu__::__always_inline__]]
> constexpr bool
> operator==(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1194,7 +1214,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() == __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - [[nodiscard]]
> + [[nodiscard, __gnu__::__always_inline__]]
> constexpr std::__detail::__synth3way_t<_Iterator>
> operator<=>(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1203,7 +1223,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> #else
> // Forward iterator requirements
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator==(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1211,7 +1231,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() == __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator==(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1219,7 +1239,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() == __rhs.base(); }
>
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator!=(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1227,7 +1247,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() != __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator!=(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1236,7 +1256,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> // Random access iterator requirements
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator<(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1244,7 +1264,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() < __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> _GLIBCXX20_CONSTEXPR
> inline bool
> operator<(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1252,7 +1272,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() < __rhs.base(); }
>
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator>(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1260,7 +1280,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() > __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator>(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1268,7 +1288,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() > __rhs.base(); }
>
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator<=(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1276,7 +1296,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() <= __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator<=(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1284,7 +1304,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() <= __rhs.base(); }
>
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> - _GLIBCXX_NODISCARD
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator>=(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs)
> @@ -1292,7 +1312,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() >= __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline bool
> operator>=(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1307,8 +1327,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> #if __cplusplus >= 201103L
> // DR 685.
> - [[__nodiscard__]] _GLIBCXX20_CONSTEXPR
> - inline auto
> + [[__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())
> @@ -1320,7 +1340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() - __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline typename __normal_iterator<_Iterator, _Container>::difference_type
> operator-(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> @@ -1328,7 +1348,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __lhs.base() - __rhs.base(); }
>
> template<typename _Iterator, typename _Container>
> - _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD _GLIBCXX_CONSTEXPR
> inline __normal_iterator<_Iterator, _Container>
> operator+(typename __normal_iterator<_Iterator,
> _Container>::difference_type
> __n, const __normal_iterator<_Iterator, _Container>& __i)
> @@ -1349,24 +1369,26 @@ namespace std _GLIBCXX_VISIBILITY(default)
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> // Unwrap a __normal_iterator to get the underlying iterator
> - // (usually a pointer)
> + // (usually a pointer). See uses in std::copy, std::fill, etc.
> template<typename _Iterator, typename _Container>
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> _GLIBCXX20_CONSTEXPR
> - _Iterator
> + inline _Iterator
> __niter_base(__gnu_cxx::__normal_iterator<_Iterator, _Container> __it)
>
> _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_copy_constructible<_Iterator>::value)
> { return __it.base(); }
>
> - // Fallback implementation of the function in bits/stl_iterator.h used to
> - // remove the __normal_iterator wrapper. See std::copy, std::fill, etc.
> + // Fallback implementation used for iterators that can't be unwrapped.
> template<typename _Iterator>
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> _GLIBCXX20_CONSTEXPR
> inline _Iterator
> __niter_base(_Iterator __it)
>
> _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_copy_constructible<_Iterator>::value)
> { return __it; }
>
> - // Overload for _Safe_iterator needs to be declared before __niter_base
> uses.
> + // Overload for _Safe_iterator needs to be declared before uses of
> + // std::__niter_base because we call it qualified so isn't found by ADL.
I guess this comment adjustment could be part of the first patch?
LGTM otherwise. Patches 5,6,7 LGTM too.
> #if __cplusplus < 201103L
> template<typename _Ite, typename _Seq>
> _Ite
> @@ -1391,6 +1413,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // __normal_iterator back again (this assumes that __normal_iterator
> // is only used to wrap random access iterators, like pointers).
> template<typename _From, typename _To>
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> _GLIBCXX20_CONSTEXPR
> inline _From
> __niter_wrap(_From __from, _To __res)
> @@ -1398,6 +1421,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> // No need to wrap, iterator already has the right type.
> template<typename _Iterator>
> + __attribute__((__always_inline__)) _GLIBCXX_NODISCARD
> _GLIBCXX20_CONSTEXPR
> inline _Iterator
> __niter_wrap(const _Iterator&, _Iterator __res)
> @@ -1407,6 +1431,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // Need to overload __to_address because the pointer_traits primary
> template
> // will deduce element_type of __normal_iterator<T*, C> as T* rather than
> T.
> template<typename _Iterator, typename _Container>
> + [[__gnu__::__always_inline__]] _GLIBCXX_NODISCARD
> constexpr auto
> __to_address(const __gnu_cxx::__normal_iterator<_Iterator,
> _Container>& __it) noexcept
> @@ -1861,7 +1886,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> = __conditional_t<__move_if_noexcept_cond
> <typename iterator_traits<_Iterator>::value_type>::value,
> _Iterator, move_iterator<_Iterator>>>
> - inline _GLIBCXX17_CONSTEXPR _ReturnType
> + [[__nodiscard__]]
> + constexpr _ReturnType
> __make_move_if_noexcept_iterator(_Iterator __i)
> { return _ReturnType(__i); }
>
> @@ -1870,13 +1896,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _Tp, typename _ReturnType
> = __conditional_t<__move_if_noexcept_cond<_Tp>::value,
> const _Tp*, move_iterator<_Tp*>>>
> - inline _GLIBCXX17_CONSTEXPR _ReturnType
> + [[__nodiscard__]]
> + constexpr _ReturnType
> __make_move_if_noexcept_iterator(_Tp* __i)
> { return _ReturnType(__i); }
>
> template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> + [[__nodiscard__]]
> + constexpr auto
> __niter_base(move_iterator<_Iterator> __it)
> -> decltype(make_move_iterator(__niter_base(__it.base())))
> { return make_move_iterator(__niter_base(__it.base())); }
> @@ -1889,8 +1916,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> };
>
> template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> + [[__nodiscard__]]
> + constexpr auto
> __miter_base(move_iterator<_Iterator> __it)
> -> decltype(__miter_base(__it.base()))
> { return __miter_base(__it.base()); }
> --
> 2.46.2
>
>
