On 28/02/25 16:27 -0500, 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)?
Good point, I've removed the constraint.
+ 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?
Yes, that makes the constraint more obviously match what it does.
[[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?
Using iter_reference_t here was a bug, which is why it's been replaced
with using __iter_ref_t:
- 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
New version attached, re-testing now...
commit c686fefce3c846f184e7a98df7e95b8c8a026c4d
Author: Jonathan Wakely <jwak...@redhat.com>
AuthorDate: Fri Feb 28 21:44:41 2025
Commit: Jonathan Wakely <r...@gcc.gnu.org>
CommitDate: Fri Feb 28 21:44:41 2025
libstdc++: Fix ranges::iter_move handling of rvalues [PR106612]
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.
diff --git a/libstdc++-v3/include/bits/iterator_concepts.h b/libstdc++-v3/include/bits/iterator_concepts.h
index 4265c475273..a201e24d2e2 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>
+ 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 { typename __iter_ref_t<_Tp>; }
[[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>>)
- 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();
}
