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

[Jonathan Wakely]

On Fri, 28 Feb 2025 at 21:47, Patrick Palka <ppalka@redhat.com> wrote:
>
> On Fri, 28 Feb 2025, Jonathan Wakely wrote:
>
> > 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...
>
> LGTM

Thanks, pushed now (and the other two you reviewed).

I'll backport all three after letting them rest on trunk for a while.