On Tue, 28 Jan 2025 at 16:00, Patrick Palka <[email protected]> wrote:
>
> On Mon, 27 Jan 2025, Patrick Palka wrote:
>
> > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
> >
> > -- >8 --
> >
> > This case was incorrectly failing in C++23 mode even after P2492R2
> > because the perfect forwarding simplification mechanism assumed bound
> > arguments are copy-constructible which is no longer necessarily true
> > after that paper.
>
> On closer inspection this isn't quite right: the mechanism has for a
> while intended to check for copy-constructibility of the bound arguments
> (even before P2492R2), but it used the wrong predicate:
> is_trivially_copyable instead of is_trivially_copy_constructible.
> Here's a simpler fix that's more suitable for backporting.
>
> Disabling the mechanism outright in C++23 mode and relying on deducing
> "this" is still desirable but I reckon that's more stage 1 material.
>
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
OK for both, thanks.
>
> -- >8 --
>
> Subject: [PATCH] libstdc++: Fix views::transform(move_only_fn{}) forwarding
> [PR118413]
>
> The range adaptor perfect forwarding simplification mechanism is currently
> only enabled for trivially copyable bound arguments, to prevent undesirable
> copies of complex objects. But "trivially copyable" is the wrong property
> to check for here, since a move-only type with a trivial move constructor
> is considered trivially copyable, and after P2492R2 we can't assume that the
> bound arguments are copy-constructible. This patch makes the mechanism
> more specifically check for trivial copy constructibility instead so that
> it's properly disabled for move-only bound arguments.
>
> PR libstdc++/118413
>
> libstdc++-v3/ChangeLog:
>
> * include/std/ranges (views::__adaptor::_Partial): Refine
> constraints on the "simple" partial specializations to require
> is_trivially_copy_constructible_v instead of
> is_trivially_copyable_v.
> * testsuite/std/ranges/adaptors/adjacent_transform/1.cc (test04):
> Test partial application of a move-only function as well.
> * testsuite/std/ranges/adaptors/transform.cc (test09): Likewise.
> ---
> libstdc++-v3/include/std/ranges | 4 ++--
> .../testsuite/std/ranges/adaptors/adjacent_transform/1.cc | 1 +
> libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc | 2 ++
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index ad69a94b21f..5c795a90fbc 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -1145,7 +1145,7 @@ namespace views::__adaptor
> // which makes overload resolution failure diagnostics more concise.
> template<typename _Adaptor, typename... _Args>
> requires __adaptor_has_simple_extra_args<_Adaptor, _Args...>
> - && (is_trivially_copyable_v<_Args> && ...)
> + && (is_trivially_copy_constructible_v<_Args> && ...)
> struct _Partial<_Adaptor, _Args...> :
> _RangeAdaptorClosure<_Partial<_Adaptor, _Args...>>
> {
> tuple<_Args...> _M_args;
> @@ -1176,7 +1176,7 @@ namespace views::__adaptor
> // where _Adaptor accepts a single extra argument.
> template<typename _Adaptor, typename _Arg>
> requires __adaptor_has_simple_extra_args<_Adaptor, _Arg>
> - && is_trivially_copyable_v<_Arg>
> + && is_trivially_copy_constructible_v<_Arg>
> struct _Partial<_Adaptor, _Arg> :
> _RangeAdaptorClosure<_Partial<_Adaptor, _Arg>>
> {
> _Arg _M_arg;
> diff --git
> a/libstdc++-v3/testsuite/std/ranges/adaptors/adjacent_transform/1.cc
> b/libstdc++-v3/testsuite/std/ranges/adaptors/adjacent_transform/1.cc
> index a5791b3da70..772e4b3b6a0 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/adjacent_transform/1.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/adjacent_transform/1.cc
> @@ -110,6 +110,7 @@ test04()
> };
> // P2494R2 Relaxing range adaptors to allow for move only types
> static_assert( requires { views::pairwise_transform(x, move_only{}); } );
> + static_assert( requires { x | views::pairwise_transform(move_only{}); } );
> }
>
> int
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> b/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> index dfc91fb5e1f..934d2f65dcf 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> @@ -191,8 +191,10 @@ test09()
> #if __cpp_lib_ranges >= 202207L
> // P2494R2 Relaxing range adaptors to allow for move only types
> static_assert( requires { transform(x, move_only{}); } );
> + static_assert( requires { x | transform(move_only{}); } ); // PR
> libstdc++/118413
> #else
> static_assert( ! requires { transform(x, move_only{}); } );
> + static_assert( ! requires { x | transform(move_only{}); } );
> #endif
> }
>
> --
> 2.48.1.91.g5f8f7081f7
>
>
> > It'd be easy enough to fix the mechanism, but in
> > C++23 mode the mechanism is no longer useful since we can just rely on
> > deducing 'this' to implement perfect forwarding with a single overload
> > (and done in r14-7150-gd2cb4693a0b383). So this patch just disables
> > the mechanism in C++23 mode so that the generic implementation is always
> > used.
> >
> > PR libstdc++/118413
> >
> > libstdc++-v3/ChangeLog:
> >
> > * include/std/ranges
> > (_GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING): New.
> > (__closure_has_simple_call_op): Only define in C++20 mode.
> > (__closure_has_simple_extra_args): Likewise.
> > (_Partial, _Pipe): Likewise, for the "simple" partial
> > specializations.
> > (*::_S_has_simple_call_op): Likewise.
> > (*::_S_has_simple_extra_args): Likewise.
> > * testsuite/std/ranges/adaptors/100577.cc: Disable some
> > implementation detail checks in C++23 mode.
> > * testsuite/std/ranges/adaptors/transform.cc (test09): Also test
> > partially applying the move-only function.
> > ---
> > libstdc++-v3/include/std/ranges | 53 +++++++++++++++++++
> > .../testsuite/std/ranges/adaptors/100577.cc | 4 +-
> > .../std/ranges/adaptors/transform.cc | 2 +
> > 3 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/libstdc++-v3/include/std/ranges
> > b/libstdc++-v3/include/std/ranges
> > index ad69a94b21f..a2b8748bea6 100644
> > --- a/libstdc++-v3/include/std/ranges
> > +++ b/libstdc++-v3/include/std/ranges
> > @@ -1028,6 +1028,16 @@ namespace views::__adaptor
> > }
> > };
> >
> > +#if __cplusplus <= 202003L
> > + // In C++20 mode we simplify perfect forwarding of a range adaptor
> > closure's
> > + // bound arguments when possible (according to their types), for sake of
> > compile
> > + // times and diagnostic quality. In C++23 mode we instead rely on
> > deducing 'this'
> > + // to idiomatically implement perfect forwarding. Note that this means
> > the
> > + // simplification logic doesn't consider C++23 <ranges> changes such as
> > P2492R2.
> > +# define _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING 1
> > +#endif
> > +
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // True if the range adaptor closure _Adaptor has a simple operator(),
> > i.e.
> > // one that's not overloaded according to constness or value category of
> > the
> > // _Adaptor object.
> > @@ -1039,6 +1049,7 @@ namespace views::__adaptor
> > template<typename _Adaptor, typename... _Args>
> > concept __adaptor_has_simple_extra_args =
> > _Adaptor::_S_has_simple_extra_args
> > || _Adaptor::template _S_has_simple_extra_args<_Args...>;
> > +#endif
> >
> > // A range adaptor closure that represents partial application of
> > // the range adaptor _Adaptor with arguments _Args.
> > @@ -1139,6 +1150,7 @@ namespace views::__adaptor
> > #endif
> > };
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // Partial specialization of the primary template for the case where the
> > extra
> > // arguments of the adaptor can always be safely and efficiently
> > forwarded by
> > // const reference. This lets us get away with a single operator()
> > overload,
> > @@ -1195,6 +1207,7 @@ namespace views::__adaptor
> >
> > static constexpr bool _S_has_simple_call_op = true;
> > };
> > +#endif // _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> >
> > template<typename _Lhs, typename _Rhs, typename _Range>
> > concept __pipe_invocable
> > @@ -1245,6 +1258,7 @@ namespace views::__adaptor
> > #endif
> > };
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // A partial specialization of the above primary template for the case
> > where
> > // both adaptor operands have a simple operator(). This in turn lets us
> > // implement composition using a single simple operator(), which makes
> > @@ -1271,6 +1285,7 @@ namespace views::__adaptor
> >
> > static constexpr bool _S_has_simple_call_op = true;
> > };
> > +#endif // _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > } // namespace views::__adaptor
> >
> > #if __cpp_lib_ranges >= 202202L
> > @@ -1454,7 +1469,9 @@ namespace views::__adaptor
> > return owning_view{std::forward<_Range>(__r)};
> > }
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_call_op = true;
> > +#endif
> > };
> >
> > inline constexpr _All all;
> > @@ -1872,7 +1889,9 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_Filter>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _Filter filter;
> > @@ -2260,7 +2279,9 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_Transform>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _Transform transform;
> > @@ -2494,12 +2515,14 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_Take>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // The count argument of views::take is not always simple -- it can
> > be
> > // e.g. a move-only class that's implicitly convertible to the
> > difference
> > // type. But an integer-like count argument is surely simple.
> > template<typename _Tp>
> > static constexpr bool _S_has_simple_extra_args
> > = ranges::__detail::__is_integer_like<_Tp>;
> > +#endif
> > };
> >
> > inline constexpr _Take take;
> > @@ -2621,7 +2644,9 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_TakeWhile>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _TakeWhile take_while;
> > @@ -2777,9 +2802,11 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_Drop>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > template<typename _Tp>
> > static constexpr bool _S_has_simple_extra_args
> > = _Take::_S_has_simple_extra_args<_Tp>;
> > +#endif
> > };
> >
> > inline constexpr _Drop drop;
> > @@ -2866,7 +2893,9 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_DropWhile>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _DropWhile drop_while;
> > @@ -3273,7 +3302,9 @@ namespace views::__adaptor
> > return join_view<all_t<_Range>>{std::forward<_Range>(__r)};
> > }
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_call_op = true;
> > +#endif
> > };
> >
> > inline constexpr _Join join;
> > @@ -3730,6 +3761,7 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_LazySplit>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // The pattern argument of views::lazy_split is not always simple --
> > it can be
> > // a non-view range, the value category of which affects whether the
> > call
> > // is well-formed. But a scalar or a view pattern argument is surely
> > @@ -3738,6 +3770,7 @@ namespace views::__adaptor
> > static constexpr bool _S_has_simple_extra_args
> > = is_scalar_v<_Pattern> || (view<_Pattern>
> > && copy_constructible<_Pattern>);
> > +#endif
> > };
> >
> > inline constexpr _LazySplit lazy_split;
> > @@ -3937,9 +3970,11 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_Split>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > template<typename _Pattern>
> > static constexpr bool _S_has_simple_extra_args
> > = _LazySplit::_S_has_simple_extra_args<_Pattern>;
> > +#endif
> > };
> >
> > inline constexpr _Split split;
> > @@ -4074,7 +4109,9 @@ namespace views::__adaptor
> > return common_view{std::forward<_Range>(__r)};
> > }
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_call_op = true;
> > +#endif
> > };
> >
> > inline constexpr _Common common;
> > @@ -4207,7 +4244,9 @@ namespace views::__adaptor
> > return reverse_view{std::forward<_Range>(__r)};
> > }
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_call_op = true;
> > +#endif
> > };
> >
> > inline constexpr _Reverse reverse;
> > @@ -4598,7 +4637,9 @@ namespace views::__adaptor
> > return elements_view<all_t<_Range>,
> > _Nm>{std::forward<_Range>(__r)};
> > }
> >
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_call_op = true;
> > +#endif
> > };
> >
> > template<size_t _Nm>
> > @@ -6061,7 +6102,9 @@ namespace views::__adaptor
> >
> > using __adaptor::_RangeAdaptor<_AdjacentTransform>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > template<size_t _Nm>
> > @@ -6617,7 +6660,9 @@ namespace views::__adaptor
> >
> > using __adaptor::_RangeAdaptor<_Chunk>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _Chunk chunk;
> > @@ -6992,7 +7037,9 @@ namespace views::__adaptor
> >
> > using __adaptor::_RangeAdaptor<_Slide>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _Slide slide;
> > @@ -7187,7 +7234,9 @@ namespace views::__adaptor
> >
> > using __adaptor::_RangeAdaptor<_ChunkBy>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _ChunkBy chunk_by;
> > @@ -7715,9 +7764,11 @@ namespace views::__adaptor
> >
> > using _RangeAdaptor<_JoinWith>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > template<typename _Pattern>
> > static constexpr bool _S_has_simple_extra_args
> > = _LazySplit::_S_has_simple_extra_args<_Pattern>;
> > +#endif
> > };
> >
> > inline constexpr _JoinWith join_with;
> > @@ -8333,7 +8384,9 @@ namespace views::__adaptor
> >
> > using __adaptor::_RangeAdaptor<_Stride>::operator();
> > static constexpr int _S_arity = 2;
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > static constexpr bool _S_has_simple_extra_args = true;
> > +#endif
> > };
> >
> > inline constexpr _Stride stride;
> > diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> > b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> > index 53c81be7f02..eaa8454b318 100644
> > --- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> > +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> > @@ -31,6 +31,7 @@ namespace views = std::ranges::views;
> > void
> > test01()
> > {
> > +#ifdef _GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING
> > // Verify adaptors are deemed to have simple extra arguments when
> > appropriate.
> > using views::__adaptor::__adaptor_has_simple_extra_args;
> > using std::identity;
> > @@ -78,6 +79,7 @@ test01()
> > static_assert(!__closure_has_simple_call_op<decltype(a12a | a00)>);
> > static_assert(!__closure_has_simple_call_op<decltype(a00 | a12a)>);
> > #endif
> > +#endif
> > }
> >
> > void
> > @@ -135,7 +137,7 @@ test03()
> > void
> > test04()
> > {
> > -#if __STDC_HOSTED__
> > +#if defined(_GLIBCXX_RANGES_SIMPLIFY_ADAPTOR_FORWARDING) && __STDC_HOSTED__
> > // Non-trivially-copyable extra arguments make a closure not simple.
> > using F = std::function<bool(bool)>;
> > static_assert(!std::is_trivially_copyable_v<F>);
> > diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> > b/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> > index dfc91fb5e1f..934d2f65dcf 100644
> > --- a/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> > +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/transform.cc
> > @@ -191,8 +191,10 @@ test09()
> > #if __cpp_lib_ranges >= 202207L
> > // P2494R2 Relaxing range adaptors to allow for move only types
> > static_assert( requires { transform(x, move_only{}); } );
> > + static_assert( requires { x | transform(move_only{}); } ); // PR
> > libstdc++/118413
> > #else
> > static_assert( ! requires { transform(x, move_only{}); } );
> > + static_assert( ! requires { x | transform(move_only{}); } );
> > #endif
> > }
> >
> > --
> > 2.48.1.91.g5f8f7081f7
> >
> >
>
