[PATCH v2] libstdc++: Fix ranges::iter_move handling of rvalues [PR106612]

[Jonathan Wakely]

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...


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 8607 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250228/5a3a3014/attachment.bin>