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