On Fri, 28 Feb 2025, Patrick Palka wrote:
> 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?
Oops, just realized I was reviewing a _removed_ line! LGTM then aside
from the first two small comments.
>
> > - 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
> >
> >
>
