On Thu, 17 Oct 2024, Jonathan Wakely wrote:
> I've split this out of "Refactor std::uninitialized_{copy, fill, fill_n}"
> because this part can be done separately. Call it [PATCH -1/7] if you
> like :-)
>
> This fixes the ordering problem that Patrick noticed in [PATCH 1/7], and
> adds a test for it. It also updates the comments as was previously done
> in [PATCH 2/7], which Patrick noted could have been done when moving the
> functions into stl_iterator.h.
LGTM
>
> Note that the __niter_base overloads for reverse_iterator and
> move_iterator call __niter_base unqualified, which means that in
> contrast to all other uses of __niter_base they *do* use ADL to find the
> next __niter_base to call. I think that's necessary so that it works for
> both reverse_iterator<move_iterator<I>> and the inverse order,
> move_iterator<reverse_iterator<I>>. I haven't changed that here, they
> still use unqualified calls.
IIUC since the overloads' constraints are mutually recursive it'd be
kind of awkward to avoid ADL. Dunno how badly we want to avoid ADL
here, but I think one way would be to define the overloads as static
member functions and make the calls within the signature
dependently-scoped, e.g.
struct __niter_base_overloads {
template<typename _Iterator, typename _Self = __niter_base_overloads>
_GLIBCXX20_CONSTEXPR
static auto
__niter_base(reverse_iterator<_Iterator> __it)
-> decltype(__make_reverse_iterator(_Self::__niter_base(__it.base())))
{ return __make_reverse_iterator(__niter_base(__it.base())); }
template<typename _Iterator, typename _Self = __niter_base_overloads>
_GLIBCXX20_CONSTEXPR
static auto
__niter_base(move_iterator<_Iterator> __it)
-> decltype(make_move_iterator(_Self::__niter_base(__it.base())))
{ return make_move_iterator(__niter_base(__it.base())); }
...
};
>
> As a further change in this area, I think it would be possible (and
> maybe nice) to remove __miter_base and replace the uses of it in
> std::move_backward(I,I,O) and std::move(I,I,O). That's left for another
> day.
>
> Tested x86_64-linux.
>
> -- >8 --
>
> Move the functions for unwrapping and rewrapping __normal_iterator
> objects to the same file as the definition of __normal_iterator itself.
>
> This will allow a later commit to make use of std::__niter_base in other
> headers without having to include all of <bits/stl_algobase.h>.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/stl_algobase.h (__niter_base, __niter_wrap): Move
> to ...
> * include/bits/stl_iterator.h: ... here.
> (__niter_base, __miter_base): Move all overloads to the end of
> the header.
> * testsuite/24_iterators/normal_iterator/wrapping.cc: New test.
> ---
> libstdc++-v3/include/bits/stl_algobase.h | 45 ------
> libstdc++-v3/include/bits/stl_iterator.h | 138 +++++++++++++-----
> .../24_iterators/normal_iterator/wrapping.cc | 29 ++++
> 3 files changed, 132 insertions(+), 80 deletions(-)
> create mode 100644
> libstdc++-v3/testsuite/24_iterators/normal_iterator/wrapping.cc
>
> diff --git a/libstdc++-v3/include/bits/stl_algobase.h
> b/libstdc++-v3/include/bits/stl_algobase.h
> index 384e5fdcdc9..751b7ad119b 100644
> --- a/libstdc++-v3/include/bits/stl_algobase.h
> +++ b/libstdc++-v3/include/bits/stl_algobase.h
> @@ -308,51 +308,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return __a;
> }
>
> - // Fallback implementation of the function in bits/stl_iterator.h used to
> - // remove the __normal_iterator wrapper. See copy, fill, ...
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - inline _Iterator
> - __niter_base(_Iterator __it)
> -
> _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_copy_constructible<_Iterator>::value)
> - { return __it; }
> -
> -#if __cplusplus < 201103L
> - template<typename _Ite, typename _Seq>
> - _Ite
> - __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
> - std::random_access_iterator_tag>&);
> -
> - template<typename _Ite, typename _Cont, typename _Seq>
> - _Ite
> - __niter_base(const ::__gnu_debug::_Safe_iterator<
> - ::__gnu_cxx::__normal_iterator<_Ite, _Cont>, _Seq,
> - std::random_access_iterator_tag>&);
> -#else
> - template<typename _Ite, typename _Seq>
> - _GLIBCXX20_CONSTEXPR
> - decltype(std::__niter_base(std::declval<_Ite>()))
> - __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
> - std::random_access_iterator_tag>&)
> - noexcept(std::is_nothrow_copy_constructible<_Ite>::value);
> -#endif
> -
> - // Reverse the __niter_base transformation to get a
> - // __normal_iterator back again (this assumes that __normal_iterator
> - // is only used to wrap random access iterators, like pointers).
> - template<typename _From, typename _To>
> - _GLIBCXX20_CONSTEXPR
> - inline _From
> - __niter_wrap(_From __from, _To __res)
> - { return __from + (std::__niter_base(__res) -
> std::__niter_base(__from)); }
> -
> - // No need to wrap, iterator already has the right type.
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - inline _Iterator
> - __niter_wrap(const _Iterator&, _Iterator __res)
> - { return __res; }
> -
> // All of these auxiliary structs serve two purposes. (1) Replace
> // calls to copy with memmove whenever possible. (Memmove, not memcpy,
> // because the input and output ranges are permitted to overlap.)
> diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> b/libstdc++-v3/include/bits/stl_iterator.h
> index 28a600c81cb..be3fa6f7a34 100644
> --- a/libstdc++-v3/include/bits/stl_iterator.h
> +++ b/libstdc++-v3/include/bits/stl_iterator.h
> @@ -654,24 +654,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> # endif // C++20
> # endif // __glibcxx_make_reverse_iterator
>
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> - __niter_base(reverse_iterator<_Iterator> __it)
> - -> decltype(__make_reverse_iterator(__niter_base(__it.base())))
> - { return __make_reverse_iterator(__niter_base(__it.base())); }
> -
> template<typename _Iterator>
> struct __is_move_iterator<reverse_iterator<_Iterator> >
> : __is_move_iterator<_Iterator>
> { };
> -
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> - __miter_base(reverse_iterator<_Iterator> __it)
> - -> decltype(__make_reverse_iterator(__miter_base(__it.base())))
> - { return __make_reverse_iterator(__miter_base(__it.base())); }
> #endif // C++11
>
> // 24.4.2.2.1 back_insert_iterator
> @@ -1336,19 +1322,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { return __normal_iterator<_Iterator, _Container>(__i.base() + __n); }
>
> _GLIBCXX_END_NAMESPACE_VERSION
> -} // namespace
> +} // namespace __gnu_cxx
>
> namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> - template<typename _Iterator, typename _Container>
> - _GLIBCXX20_CONSTEXPR
> - _Iterator
> - __niter_base(__gnu_cxx::__normal_iterator<_Iterator, _Container> __it)
> -
> _GLIBCXX_NOEXCEPT_IF(std::is_nothrow_copy_constructible<_Iterator>::value)
> - { return __it.base(); }
> -
> #if __cplusplus >= 201103L && __cplusplus <= 201703L
> // 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.
> @@ -1820,13 +1799,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> __make_move_if_noexcept_iterator(_Tp* __i)
> { return _ReturnType(__i); }
>
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> - __niter_base(move_iterator<_Iterator> __it)
> - -> decltype(make_move_iterator(__niter_base(__it.base())))
> - { return make_move_iterator(__niter_base(__it.base())); }
> -
> template<typename _Iterator>
> struct __is_move_iterator<move_iterator<_Iterator> >
> {
> @@ -1834,12 +1806,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> typedef __true_type __type;
> };
>
> - template<typename _Iterator>
> - _GLIBCXX20_CONSTEXPR
> - auto
> - __miter_base(move_iterator<_Iterator> __it)
> - -> decltype(__miter_base(__it.base()))
> - { return __miter_base(__it.base()); }
>
> #define _GLIBCXX_MAKE_MOVE_ITERATOR(_Iter) std::make_move_iterator(_Iter)
> #define _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR(_Iter) \
> @@ -2985,6 +2951,108 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> /// @} group iterators
>
> +_GLIBCXX_END_NAMESPACE_VERSION
> +} // namespace std
> +
> +namespace __gnu_debug
> +{
> + template<typename _Iterator, typename _Sequence, typename _Category>
> + class _Safe_iterator;
> +}
> +
> +namespace std _GLIBCXX_VISIBILITY(default)
> +{
> +_GLIBCXX_BEGIN_NAMESPACE_VERSION
> + /// @cond undocumented
> +
> + // Unwrap a __normal_iterator to get the underlying iterator
> + // (usually a pointer). See uses in std::copy, std::fill, etc.
> + template<typename _Iterator, typename _Container>
> + _GLIBCXX20_CONSTEXPR
> + _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 used for iterators that can't be unwrapped.
> + template<typename _Iterator>
> + _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 uses of
> + // std::__niter_base because we call it qualified so isn't found by ADL.
> +#if __cplusplus < 201103L
> + template<typename _Ite, typename _Seq>
> + _Ite
> + __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
> + std::random_access_iterator_tag>&);
> +
> + template<typename _Ite, typename _Cont, typename _Seq>
> + _Ite
> + __niter_base(const ::__gnu_debug::_Safe_iterator<
> + ::__gnu_cxx::__normal_iterator<_Ite, _Cont>, _Seq,
> + std::random_access_iterator_tag>&);
> +#else
> + template<typename _Ite, typename _Seq>
> + _GLIBCXX20_CONSTEXPR
> + decltype(std::__niter_base(std::declval<_Ite>()))
> + __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
> + std::random_access_iterator_tag>&)
> + noexcept(std::is_nothrow_copy_constructible<_Ite>::value);
> +#endif
> +
> +#if __cplusplus >= 201103L
> + template<typename _Iterator>
> + _GLIBCXX20_CONSTEXPR
> + auto
> + __niter_base(reverse_iterator<_Iterator> __it)
> + -> decltype(__make_reverse_iterator(__niter_base(__it.base())))
> + { return __make_reverse_iterator(__niter_base(__it.base())); }
> +
> + template<typename _Iterator>
> + _GLIBCXX20_CONSTEXPR
> + auto
> + __niter_base(move_iterator<_Iterator> __it)
> + -> decltype(make_move_iterator(__niter_base(__it.base())))
> + { return make_move_iterator(__niter_base(__it.base())); }
> +
> + template<typename _Iterator>
> + _GLIBCXX20_CONSTEXPR
> + auto
> + __miter_base(reverse_iterator<_Iterator> __it)
> + -> decltype(__make_reverse_iterator(__miter_base(__it.base())))
> + { return __make_reverse_iterator(__miter_base(__it.base())); }
> +
> + template<typename _Iterator>
> + _GLIBCXX20_CONSTEXPR
> + auto
> + __miter_base(move_iterator<_Iterator> __it)
> + -> decltype(__miter_base(__it.base()))
> + { return __miter_base(__it.base()); }
> +#endif
> +
> + // Reverse the __niter_base transformation to get a __normal_iterator
> + // back again (this assumes that __normal_iterator is only used to wrap
> + // random access iterators, like pointers).
> + // All overloads of std::__niter_base must be declared before this.
> + template<typename _From, typename _To>
> + _GLIBCXX20_CONSTEXPR
> + inline _From
> + __niter_wrap(_From __from, _To __res)
> + { return __from + (std::__niter_base(__res) -
> std::__niter_base(__from)); }
> +
> + // No need to wrap, iterator already has the right type.
> + template<typename _Iterator>
> + _GLIBCXX20_CONSTEXPR
> + inline _Iterator
> + __niter_wrap(const _Iterator&, _Iterator __res)
> + { return __res; }
> +
> + /// @endcond
> +
> #if __cpp_deduction_guides >= 201606
> // These helper traits are used for deduction guides
> // of associative containers.
> diff --git a/libstdc++-v3/testsuite/24_iterators/normal_iterator/wrapping.cc
> b/libstdc++-v3/testsuite/24_iterators/normal_iterator/wrapping.cc
> new file mode 100644
> index 00000000000..bbfd4264a36
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/24_iterators/normal_iterator/wrapping.cc
> @@ -0,0 +1,29 @@
> +#include <iterator>
> +#include <algorithm>
> +#include <vector>
> +#ifndef _GLIBCXX_DEBUG
> +#include <debug/vector>
> +#endif
> +
> +struct S { };
> +
> +int main()
> +{
> + S s[1];
> + std::vector<S> v(1);
> + std::copy(s, s, v.rbegin());
> +#if __cplusplus >= 201103L
> + std::copy(s, s, std::make_move_iterator(v.begin()));
> + std::copy(s, s, std::make_move_iterator(v.rbegin()));
> +#endif
> +
> +#ifndef _GLIBCXX_DEBUG
> + __gnu_debug::vector<S> dv(1);
> + std::copy(s, s, dv.rbegin());
> +#if __cplusplus >= 201103L
> + std::copy(s, s, std::make_move_iterator(dv.begin()));
> + std::copy(s, s, std::make_move_iterator(dv.rbegin()));
> +#endif
> +#endif
> +}
> +
> --
> 2.46.2
>
>
