On Thu, 27 Feb 2025, Jonathan Wakely wrote:
> The specification for std::ranges::iter_move apparently requires us to
> handle types which do not satisfy std::indirectly_readable, for example
> with overloaded operator* which behaves differently for different value
> categories.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/106612
> * include/bits/iterator_concepts.h (_IterMove::__iter_ref_t):
> New alias template.
> (_IterMove::__result): Use __iter_ref_t instead of
> std::iter_reference_t.
> (_IterMove::__type): Remove incorrect __dereferenceable
> constraint.
> (_IterMove::operator()): Likewise. Add correct constraints. Use
> __iter_ref_t instead of std::iter_reference_t. Forward parameter
> as correct value category.
> (iter_swap): Add comments.
> * testsuite/24_iterators/customization_points/iter_move.cc: Test
> that iter_move is found by ADL and that rvalue arguments are
> handled correctly.
> ---
>
> Tested x86_64-linux.
>
> I think the spec is silly to require this, but here we are.
>
> libstdc++-v3/include/bits/iterator_concepts.h | 33 +++++--
> .../customization_points/iter_move.cc | 95 +++++++++++++++++++
> 2 files changed, 119 insertions(+), 9 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/iterator_concepts.h
> b/libstdc++-v3/include/bits/iterator_concepts.h
> index 4265c475273..555af3bdb38 100644
> --- a/libstdc++-v3/include/bits/iterator_concepts.h
> +++ b/libstdc++-v3/include/bits/iterator_concepts.h
> @@ -103,32 +103,42 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> namespace ranges
> {
> /// @cond undocumented
> + // Implementation of std::ranges::iter_move, [iterator.cust.move].
> namespace __imove
> {
> void iter_move() = delete;
>
> + // Satisfied if _Tp is a class or enumeration type and iter_move
> + // can be found by argument-dependent lookup.
> template<typename _Tp>
> concept __adl_imove
> = (std::__detail::__class_or_enum<remove_reference_t<_Tp>>)
> - && requires(_Tp&& __t) { iter_move(static_cast<_Tp&&>(__t)); };
> + && requires(_Tp&& __t) { iter_move(static_cast<_Tp&&>(__t)); };
>
> struct _IterMove
> {
> private:
> + // The type returned by dereferencing a value of type _Tp.
> + // Unlike iter_reference_t this preserves the value category of _Tp.
> + template<typename _Tp> requires requires { *std::declval<_Tp>(); }
IIUC this requires-clause is redundant since it's checking a subset
of the alias definition (and alias templates are transparent so if the
decltype is invalid it'll induce an error in the immediate context)?
> + using __iter_ref_t = decltype(*std::declval<_Tp>());
> +
> template<typename _Tp>
> struct __result
> - { using type = iter_reference_t<_Tp>; };
> + { using type = __iter_ref_t<_Tp>; };
>
> + // Use iter_move(E) if that works.
> template<typename _Tp>
> requires __adl_imove<_Tp>
> struct __result<_Tp>
> { using type = decltype(iter_move(std::declval<_Tp>())); };
>
> + // Otherwise, if *E if an lvalue, use std::move(*E).
> template<typename _Tp>
> requires (!__adl_imove<_Tp>)
> - && is_lvalue_reference_v<iter_reference_t<_Tp>>
> + && is_lvalue_reference_v<__iter_ref_t<_Tp>>
> struct __result<_Tp>
> - { using type = remove_reference_t<iter_reference_t<_Tp>>&&; };
> + { using type = remove_reference_t<__iter_ref_t<_Tp>>&&; };
>
> template<typename _Tp>
> static constexpr bool
> @@ -142,10 +152,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> public:
> // The result type of iter_move(std::declval<_Tp>())
> - template<std::__detail::__dereferenceable _Tp>
> + template<typename _Tp>
> using __type = typename __result<_Tp>::type;
>
> - template<std::__detail::__dereferenceable _Tp>
> + template<typename _Tp>
> + requires __adl_imove<_Tp> || requires { *std::declval<_Tp>(); }
Maybe requires { typename __iter_ref_t<_Tp>; } instead to make it more
obvious that the __iter_ref_t<_Tp> in the function body is always valid?
> [[nodiscard]]
> constexpr __type<_Tp>
> operator()(_Tp&& __e) const
> @@ -153,10 +164,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> {
> if constexpr (__adl_imove<_Tp>)
> return iter_move(static_cast<_Tp&&>(__e));
> - else if constexpr (is_lvalue_reference_v<iter_reference_t<_Tp>>)
With the new constraints it's not clear that iter_reference_t<_Tp> is
always valid here, say, if _Tp has an operator* that returns void?
> - return static_cast<__type<_Tp>>(*__e);
> + else if constexpr (is_lvalue_reference_v<__iter_ref_t<_Tp>>)
> + return std::move(*static_cast<_Tp&&>(__e));
> else
> - return *__e;
> + return *static_cast<_Tp&&>(__e);
> }
> };
> } // namespace __imove
> @@ -167,6 +178,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> }
> } // namespace ranges
>
> + /// The result type of ranges::iter_move(std::declval<_Tp&>())
> template<__detail::__dereferenceable _Tp>
> requires
> __detail::__can_reference<ranges::__imove::_IterMove::__type<_Tp&>>
> using iter_rvalue_reference_t = ranges::__imove::_IterMove::__type<_Tp&>;
> @@ -873,11 +885,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> namespace ranges
> {
> /// @cond undocumented
> + // Implementation of std::ranges::iter_swap, [iterator.cust.swap].
> namespace __iswap
> {
> template<typename _It1, typename _It2>
> void iter_swap(_It1, _It2) = delete;
>
> + // Satisfied if _Tp and _Up are class or enumeration types and iter_swap
> + // can be found by argument-dependent lookup.
> template<typename _Tp, typename _Up>
> concept __adl_iswap
> = (std::__detail::__class_or_enum<remove_reference_t<_Tp>>
> diff --git
> a/libstdc++-v3/testsuite/24_iterators/customization_points/iter_move.cc
> b/libstdc++-v3/testsuite/24_iterators/customization_points/iter_move.cc
> index c5def5ac577..341bd5b98d7 100644
> --- a/libstdc++-v3/testsuite/24_iterators/customization_points/iter_move.cc
> +++ b/libstdc++-v3/testsuite/24_iterators/customization_points/iter_move.cc
> @@ -63,8 +63,103 @@ test01()
> VERIFY( test_X(3, 4) );
> }
>
> +template<typename T>
> +using rval_ref = std::iter_rvalue_reference_t<T>;
> +
> +static_assert(std::same_as<rval_ref<int*>, int&&>);
> +static_assert(std::same_as<rval_ref<const int*>, const int&&>);
> +static_assert(std::same_as<rval_ref<std::move_iterator<int*>>, int&&>);
> +
> +template<typename T>
> +concept iter_movable = requires { std::ranges::iter_move(std::declval<T>());
> };
> +
> +struct Iter
> +{
> + friend int& iter_move(Iter&) { static int i = 1; return i; }
> + friend long iter_move(Iter&&) { return 2; }
> + const short& operator*() const & { static short s = 3; return s; }
> + friend float operator*(const Iter&&) { return 4.0f; }
> +};
> +
> +void
> +test_adl()
> +{
> + Iter it;
> + const Iter& cit = it;
> +
> + VERIFY( std::ranges::iter_move(it) == 1 );
> + VERIFY( std::ranges::iter_move(std::move(it)) == 2 );
> + VERIFY( std::ranges::iter_move(cit) == 3 );
> + VERIFY( std::ranges::iter_move(std::move(cit)) == 4.0f );
> +
> + // The return type should be unchanged for ADL iter_move:
> + static_assert(std::same_as<decltype(std::ranges::iter_move(it)), int&>);
> + static_assert(std::same_as<decltype(std::ranges::iter_move(std::move(it))),
> + long>);
> + // When ADL iter_move is not used, return type should be an rvalue:
> + static_assert(std::same_as<decltype(std::ranges::iter_move(cit)),
> + const short&&>);
> +
> static_assert(std::same_as<decltype(std::ranges::iter_move(std::move(cit))),
> + float>);
> +
> + // std::iter_rvalue_reference_t always considers the argument as lvalue.
> + static_assert(std::same_as<rval_ref<Iter>, int&>);
> + static_assert(std::same_as<rval_ref<Iter&>, int&>);
> + static_assert(std::same_as<rval_ref<const Iter>, const short&&>);
> + static_assert(std::same_as<rval_ref<const Iter&>, const short&&>);
> +}
> +
> +void
> +test_pr106612()
> +{
> + // Bug 106612 ranges::iter_move does not consider iterator's value
> categories
> +
> + struct I
> + {
> + int i{};
> + int& operator*() & { return i; }
> + int operator*() const & { return i; }
> + void operator*() && = delete;
> + };
> +
> + static_assert( iter_movable<I&> );
> + static_assert( iter_movable<I const&> );
> + static_assert( ! iter_movable<I> );
> + static_assert( std::same_as<std::iter_rvalue_reference_t<I>, int&&> );
> + static_assert( std::same_as<std::iter_rvalue_reference_t<const I>, int> );
> +
> + struct I2
> + {
> + int i{};
> + int& operator*() & { return i; }
> + int operator*() const & { return i; }
> + void operator*() &&;
> + };
> +
> + static_assert( iter_movable<I2&> );
> + static_assert( iter_movable<I2 const&> );
> + static_assert( iter_movable<I2> );
> + static_assert( std::is_void_v<decltype(std::ranges::iter_move(I2{}))> );
> + static_assert( std::same_as<std::iter_rvalue_reference_t<I2>, int&&> );
> + static_assert( std::same_as<std::iter_rvalue_reference_t<I2 const>, int> );
> +
> + enum E { e };
> + enum F { f };
> +
> + struct I3
> + {
> + E operator*() const & { return e; }
> + F operator*() && { return f; }
> + };
> +
> + static_assert( iter_movable<I3&> );
> + static_assert( iter_movable<I3> );
> + static_assert( std::same_as<decltype(std::ranges::iter_move(I3{})), F> );
> +}
> +
> int
> main()
> {
> test01();
> + test_adl();
> }
> --
> 2.48.1
>
>
