[PATCH] libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]

[PATCH] libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]

[Patrick Palka]

The _S_has_simple_extra_args mechanism is used to simplify forwarding
of range adaptor's extra arguments when perfect forwarding call wrapper
semantics isn't required for correctness, on a per-adaptor basis.
Both views::take and views::drop are flagged as such, but it turns out
perfect forwarding semantics are needed for these adaptors in some
contrived cases, e.g. when their extra argument is a move-only class
that's implicitly convertible to an integral type.

To fix this, we could just clear the flag for views::take/drop as with
views::split, but that'd come at the cost of acceptable diagnostics
for ill-formed uses of these adaptors (see PR100577).

This patch instead allows adaptors to parameterize their
_S_has_simple_extra_args flag according the types of the captured extra
arguments, so that we could conditionally disable perfect forwarding
semantics only when the types of the extra arguments permit it.  We
then use this finer-grained mechanism to safely disable perfect
forwarding semantics for views::take/drop when the extra argument is
integer-like, rather than incorrectly always disabling it.  Similarly,
for views::split, rather than always enabling perfect forwarding
semantics we now safely disable it when the extra argument is a scalar
or a view, and recover good diagnostics for these common cases.

Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?

	PR libstdc++/100940

libstdc++-v3/ChangeLog:

	* include/std/ranges (__adaptor::_RangeAdaptor): Document the
	template form of _S_has_simple_extra_args.
	(__adaptor::__adaptor_has_simple_extra_args): Add _Args template
	parameter pack.  Try to treat _S_has_simple_extra_args as a
	variable template parameterized by _Args.
	(__adaptor::_Partial): Pass _Arg/_Args to the constraint
	__adaptor_has_simple_extra_args.
	(views::_Take::_S_has_simple_extra_args): Templatize according
	to the type of the extra argument.
	(views::_Drop::_S_has_simple_extra_args): Likewise.
	(views::_Split::_S_has_simple_extra_args): Define.
	* testsuite/std/ranges/adaptors/100577.cc (test01, test02):
	Adjust after changes to _S_has_simple_extra_args mechanism.
	(test03): Define.
---
 libstdc++-v3/include/std/ranges               | 37 +++++++++----
 .../testsuite/std/ranges/adaptors/100577.cc   | 55 ++++++++++++-------
 2 files changed, 61 insertions(+), 31 deletions(-)

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 220a44e11a8..856975c6934 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -792,7 +792,9 @@ namespace views::__adaptor
   //
   // The optional static data member _Derived::_S_has_simple_extra_args should
   // be defined to true if the behavior of this adaptor is independent of the
-  // constness/value category of the extra arguments.
+  // constness/value category of the extra arguments.  This data member could
+  // also be defined as a variable template parameterized by the types of the
+  // extra arguments.
   template<typename _Derived>
     struct _RangeAdaptor
     {
@@ -814,9 +816,10 @@ namespace views::__adaptor
     concept __closure_has_simple_call_op = _Adaptor::_S_has_simple_call_op;
 
   // True if the behavior of the range adaptor non-closure _Adaptor is
-  // independent of the value category of its extra arguments.
-  template<typename _Adaptor>
-    concept __adaptor_has_simple_extra_args = _Adaptor::_S_has_simple_extra_args;
+  // independent of the value category of its extra arguments _Args.
+  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...>;
 
   // A range adaptor closure that represents partial application of
   // the range adaptor _Adaptor with arguments _Args.
@@ -893,7 +896,7 @@ namespace views::__adaptor
   // This lets us get away with a single operator() overload, which makes
   // overload resolution failure diagnostics more concise.
   template<typename _Adaptor, typename... _Args>
-    requires __adaptor_has_simple_extra_args<_Adaptor>
+    requires __adaptor_has_simple_extra_args<_Adaptor, _Args...>
     struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure
     {
       tuple<_Args...> _M_args;
@@ -922,7 +925,7 @@ namespace views::__adaptor
   // A lightweight specialization of the above template for the common case
   // where _Adaptor accepts a single extra argument.
   template<typename _Adaptor, typename _Arg>
-    requires __adaptor_has_simple_extra_args<_Adaptor>
+    requires __adaptor_has_simple_extra_args<_Adaptor, _Arg>
     struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure
     {
       _Arg _M_arg;
@@ -2094,7 +2097,12 @@ namespace views::__adaptor
 
       using _RangeAdaptor<_Take>::operator();
       static constexpr int _S_arity = 2;
-      static constexpr bool _S_has_simple_extra_args = true;
+      // 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>;
     };
 
     inline constexpr _Take take;
@@ -2334,7 +2342,9 @@ namespace views::__adaptor
 
       using _RangeAdaptor<_Drop>::operator();
       static constexpr int _S_arity = 2;
-      static constexpr bool _S_has_simple_extra_args = true;
+      template<typename _Tp>
+	static constexpr bool _S_has_simple_extra_args
+	  = _Take::_S_has_simple_extra_args<_Tp>;
     };
 
     inline constexpr _Drop drop;
@@ -3212,9 +3222,14 @@ namespace views::__adaptor
 
       using _RangeAdaptor<_Split>::operator();
       static constexpr int _S_arity = 2;
-      // The second argument of views::split is _not_ simple -- it can be a
-      // non-view range, the value category of which affects whether the call is
-      // well-formed.  So we must not define _S_has_simple_extra_args to true.
+      // The pattern argument of views::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
+      // simple.
+      template<typename _Pattern>
+	static constexpr bool _S_has_simple_extra_args
+	  = is_scalar_v<_Pattern> || (view<_Pattern>
+				      && copy_constructible<_Pattern>);
     };
 
     inline constexpr _Split split;
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
index 29176c8b7b0..8ef084621f9 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
@@ -28,16 +28,18 @@ namespace views = std::ranges::views;
 void
 test01()
 {
-  // Verify all multi-argument adaptors except for views::split are denoted
-  // to have simple extra arguments.
+  // Verify adaptors are deemed to have simple extra arguments when appropriate.
   using views::__adaptor::__adaptor_has_simple_extra_args;
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::transform)>);
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::filter)>);
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop)>);
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::take)>);
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::take_while)>);
-  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop_while)>);
-  static_assert(!__adaptor_has_simple_extra_args<decltype(views::split)>);
+  using std::identity;
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::transform), identity>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::filter), identity>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop), int>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::take), int>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::take_while), identity>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop_while), identity>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::split), std::string_view>);
+  static_assert(__adaptor_has_simple_extra_args<decltype(views::split), char>);
+  static_assert(!__adaptor_has_simple_extra_args<decltype(views::split), std::string>);
 
   // Verify all adaptor closures except for views::split(pattern) have a simple
   // operator().
@@ -53,15 +55,17 @@ test01()
   __closure_has_simple_call_op auto a08 = views::common;
   __closure_has_simple_call_op auto a09 = views::reverse;
   __closure_has_simple_call_op auto a10 = views::keys;
+  __closure_has_simple_call_op auto a11 = views::split(' ');
   // Verify composition of simple closures is simple.
   __closure_has_simple_call_op auto b
-    = (a00 | a01) | (a02 | a03) | (a04 | a05 | a06) | (a07 | a08 | a09 | a10);
+    = (a00 | a01) | (a02 | a03) | (a04 | a05 | a06) | (a07 | a08 | a09 | a10) | a11;
 
-  // Verify views::split is the exception.
-  auto a11 = views::split(' ');
-  static_assert(!__closure_has_simple_call_op<decltype(a11)>);
-  static_assert(!__closure_has_simple_call_op<decltype(a11 | a00)>);
-  static_assert(!__closure_has_simple_call_op<decltype(a00 | a11)>);
+  // Verify views::split(non_view_range) is an exception.
+  extern std::string s;
+  auto a12 = views::split(s);
+  static_assert(!__closure_has_simple_call_op<decltype(a12)>);
+  static_assert(!__closure_has_simple_call_op<decltype(a12 | a00)>);
+  static_assert(!__closure_has_simple_call_op<decltype(a00 | a12)>);
 }
 
 void
@@ -71,18 +75,14 @@ test02()
   // fallback deleted overload, so when a call is ill-formed overload resolution
   // fails.
   extern int x[10];
-  auto badarg = nullptr;
+  struct { } badarg;
   views::transform(badarg)(x); // { dg-error "no match" }
   views::filter(badarg)(x); // { dg-error "no match" }
-  views::take(badarg)(x); // { dg-error "no match" }
-  views::drop(badarg)(x); // { dg-error "no match" }
   views::take_while(badarg)(x); // { dg-error "no match" }
   views::drop_while(badarg)(x); // { dg-error "no match" }
 
   (views::transform(badarg) | views::all)(x); // { dg-error "no match" }
   (views::filter(badarg) | views::all)(x); // { dg-error "no match" }
-  (views::take(badarg) | views::all)(x); // { dg-error "no match" }
-  (views::drop(badarg) | views::all)(x); // { dg-error "no match" }
   (views::take_while(badarg) | views::all)(x); // { dg-error "no match" }
   (views::drop_while(badarg) | views::all)(x); // { dg-error "no match" }
 
@@ -96,6 +96,21 @@ test02()
   a0(x); // { dg-error "no match" };
   auto a1 = a0 | views::all;
   a1(x); // { dg-error "no match" }
+
+  views::take(badarg)(x); // { dg-error "deleted" }
+  views::drop(badarg)(x); // { dg-error "deleted" }
+  (views::take(badarg) | views::all)(x); // { dg-error "deleted" }
+  (views::drop(badarg) | views::all)(x); // { dg-error "deleted" }
+}
+
+void
+test03()
+{
+  // PR libstdc++/100940
+  extern int x[10];
+  struct S { operator int() && { return 5; }; };
+  x | std::views::take(S{});
+  x | std::views::drop(S{});
 }
 
 // { dg-prune-output "in requirements" }
-- 
2.32.0.93.g670b81a890

[PATCH 2/1] libstdc++: Non-triv-copyable extra args aren't simple [PR100940]

[Patrick Palka]

This force-enables perfect forwarding call wrapper semantics whenever
the extra arguments of a partially applied range adaptor aren't all
trivially copyable, so as to avoid incurring unnecessary copies of
potentially expensive-to-copy objects (such as std::function objects)
when invoking the adaptor.

Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?

	PR libstdc++/100940

libstdc++-v3/ChangeLog:

	* include/std/ranges (__adaptor::__adaptor_has_simple_extra_args): Also
	require that the extra arguments are trivially copyable.
	* testsuite/std/ranges/adaptors/100577.cc (test04): New test.
---
 libstdc++-v3/include/std/ranges               |  6 ++++--
 .../testsuite/std/ranges/adaptors/100577.cc   | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 856975c6934..e858df88088 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -818,8 +818,10 @@ namespace views::__adaptor
   // True if the behavior of the range adaptor non-closure _Adaptor is
   // independent of the value category of its extra arguments _Args.
   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...>;
+    concept __adaptor_has_simple_extra_args
+      = (_Adaptor::_S_has_simple_extra_args
+	 || _Adaptor::template _S_has_simple_extra_args<_Args...>)
+	&& (is_trivially_copyable_v<_Args> && ...);
 
   // A range adaptor closure that represents partial application of
   // the range adaptor _Adaptor with arguments _Args.
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
index 8ef084621f9..4040f474ad9 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
@@ -21,6 +21,7 @@
 // PR libstdc++/100577
 
 #include <ranges>
+#include <functional>
 
 namespace ranges = std::ranges;
 namespace views = std::ranges::views;
@@ -113,4 +114,22 @@ test03()
   x | std::views::drop(S{});
 }
 
+void
+test04()
+{
+  // Non-trivially-copyable extra arguments make a closure not simple.
+  using F = std::function<bool(bool)>;
+  static_assert(!std::is_trivially_copyable_v<F>);
+  using views::__adaptor::__adaptor_has_simple_extra_args;
+  using views::__adaptor::__closure_has_simple_call_op;
+  static_assert(!__adaptor_has_simple_extra_args<decltype(views::take_while), F>);
+  static_assert(!__adaptor_has_simple_extra_args<decltype(views::drop_while), F>);
+  static_assert(!__adaptor_has_simple_extra_args<decltype(views::filter), F>);
+  static_assert(!__adaptor_has_simple_extra_args<decltype(views::transform), F>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::take_while(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::drop_while(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::filter(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::transform(std::declval<F>()))>);
+}
+
 // { dg-prune-output "in requirements" }
-- 
2.32.0.93.g670b81a890

Re: [PATCH 2/1] libstdc++: Non-triv-copyable extra args aren't simple [PR100940]

[Patrick Palka]

On Tue, 15 Jun 2021, Patrick Palka wrote:

> This force-enables perfect forwarding call wrapper semantics whenever
> the extra arguments of a partially applied range adaptor aren't all
> trivially copyable, so as to avoid incurring unnecessary copies of
> potentially expensive-to-copy objects (such as std::function objects)
> when invoking the adaptor.
> 
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?
> 
> 	PR libstdc++/100940
> 
> libstdc++-v3/ChangeLog:
> 
> 	* include/std/ranges (__adaptor::__adaptor_has_simple_extra_args): Also
> 	require that the extra arguments are trivially copyable.
> 	* testsuite/std/ranges/adaptors/100577.cc (test04): New test.
> ---
>  libstdc++-v3/include/std/ranges               |  6 ++++--
>  .../testsuite/std/ranges/adaptors/100577.cc   | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 856975c6934..e858df88088 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -818,8 +818,10 @@ namespace views::__adaptor
>    // True if the behavior of the range adaptor non-closure _Adaptor is
>    // independent of the value category of its extra arguments _Args.
>    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...>;
> +    concept __adaptor_has_simple_extra_args
> +      = (_Adaptor::_S_has_simple_extra_args
> +	 || _Adaptor::template _S_has_simple_extra_args<_Args...>)
> +	&& (is_trivially_copyable_v<_Args> && ...);

On second thought, perhaps it'd be cleaner to leave this concept alone
and instead encode the trivial-copyability requirement as a separate
constraint on the relevant partial specializations of _Partial?
Something like:

-- >8 --


	PR libstdc++/100940

libstdc++-v3/ChangeLog:

	* include/std/ranges (__adaptor::_Partial): For the "simple"
	forwarding partial specializations, also require that
	the extra arguments are trivially copyable.
	* testsuite/std/ranges/adaptors/100577.cc (test04): New test.
---
 libstdc++-v3/include/std/ranges                    |  8 +++++---
 .../testsuite/std/ranges/adaptors/100577.cc        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 856975c6934..24411124580 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -892,11 +892,12 @@ namespace views::__adaptor
     };
 
   // Partial specialization of the primary template for the case where the extra
-  // arguments of the adaptor can always be safely forwarded by const reference.
-  // This lets us get away with a single operator() overload, which makes
-  // overload resolution failure diagnostics more concise.
+  // arguments of the adaptor can always be safely and efficiently forwarded by
+  // const reference.  This lets us get away with a single operator() overload,
+  // 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> && ...)
     struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure
     {
       tuple<_Args...> _M_args;
@@ -926,6 +927,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>
     struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure
     {
       _Arg _M_arg;
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
index 8ef084621f9..06be4980ddb 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
@@ -21,6 +21,7 @@
 // PR libstdc++/100577
 
 #include <ranges>
+#include <functional>
 
 namespace ranges = std::ranges;
 namespace views = std::ranges::views;
@@ -113,4 +114,17 @@ test03()
   x | std::views::drop(S{});
 }
 
+void
+test04()
+{
+  // Non-trivially-copyable extra arguments make a closure not simple.
+  using F = std::function<bool(bool)>;
+  static_assert(!std::is_trivially_copyable_v<F>);
+  using views::__adaptor::__closure_has_simple_call_op;
+  static_assert(!__closure_has_simple_call_op<decltype(views::take_while(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::drop_while(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::filter(std::declval<F>()))>);
+  static_assert(!__closure_has_simple_call_op<decltype(views::transform(std::declval<F>()))>);
+}
+
 // { dg-prune-output "in requirements" }
-- 
2.32.0.93.g670b81a890

Re: [PATCH 2/1] libstdc++: Non-triv-copyable extra args aren't simple [PR100940]

[Jonathan Wakely]

On Tue, 15 Jun 2021 at 20:29, Patrick Palka via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> On Tue, 15 Jun 2021, Patrick Palka wrote:
>
> > This force-enables perfect forwarding call wrapper semantics whenever
> > the extra arguments of a partially applied range adaptor aren't all
> > trivially copyable, so as to avoid incurring unnecessary copies of
> > potentially expensive-to-copy objects (such as std::function objects)
> > when invoking the adaptor.
> >
> > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?
> >
> >       PR libstdc++/100940
> >
> > libstdc++-v3/ChangeLog:
> >
> >       * include/std/ranges (__adaptor::__adaptor_has_simple_extra_args): Also
> >       require that the extra arguments are trivially copyable.
> >       * testsuite/std/ranges/adaptors/100577.cc (test04): New test.
> > ---
> >  libstdc++-v3/include/std/ranges               |  6 ++++--
> >  .../testsuite/std/ranges/adaptors/100577.cc   | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> > index 856975c6934..e858df88088 100644
> > --- a/libstdc++-v3/include/std/ranges
> > +++ b/libstdc++-v3/include/std/ranges
> > @@ -818,8 +818,10 @@ namespace views::__adaptor
> >    // True if the behavior of the range adaptor non-closure _Adaptor is
> >    // independent of the value category of its extra arguments _Args.
> >    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...>;
> > +    concept __adaptor_has_simple_extra_args
> > +      = (_Adaptor::_S_has_simple_extra_args
> > +      || _Adaptor::template _S_has_simple_extra_args<_Args...>)
> > +     && (is_trivially_copyable_v<_Args> && ...);
>
> On second thought, perhaps it'd be cleaner to leave this concept alone
> and instead encode the trivial-copyability requirement as a separate
> constraint on the relevant partial specializations of _Partial?
> Something like:

OK for trunk and 11, thanks.


>
> -- >8 --
>
>
>         PR libstdc++/100940
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/ranges (__adaptor::_Partial): For the "simple"
>         forwarding partial specializations, also require that
>         the extra arguments are trivially copyable.
>         * testsuite/std/ranges/adaptors/100577.cc (test04): New test.
> ---
>  libstdc++-v3/include/std/ranges                    |  8 +++++---
>  .../testsuite/std/ranges/adaptors/100577.cc        | 14 ++++++++++++++
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 856975c6934..24411124580 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -892,11 +892,12 @@ namespace views::__adaptor
>      };
>
>    // Partial specialization of the primary template for the case where the extra
> -  // arguments of the adaptor can always be safely forwarded by const reference.
> -  // This lets us get away with a single operator() overload, which makes
> -  // overload resolution failure diagnostics more concise.
> +  // arguments of the adaptor can always be safely and efficiently forwarded by
> +  // const reference.  This lets us get away with a single operator() overload,
> +  // 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> && ...)
>      struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure
>      {
>        tuple<_Args...> _M_args;
> @@ -926,6 +927,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>
>      struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure
>      {
>        _Arg _M_arg;
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> index 8ef084621f9..06be4980ddb 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> @@ -21,6 +21,7 @@
>  // PR libstdc++/100577
>
>  #include <ranges>
> +#include <functional>
>
>  namespace ranges = std::ranges;
>  namespace views = std::ranges::views;
> @@ -113,4 +114,17 @@ test03()
>    x | std::views::drop(S{});
>  }
>
> +void
> +test04()
> +{
> +  // Non-trivially-copyable extra arguments make a closure not simple.
> +  using F = std::function<bool(bool)>;
> +  static_assert(!std::is_trivially_copyable_v<F>);
> +  using views::__adaptor::__closure_has_simple_call_op;
> +  static_assert(!__closure_has_simple_call_op<decltype(views::take_while(std::declval<F>()))>);
> +  static_assert(!__closure_has_simple_call_op<decltype(views::drop_while(std::declval<F>()))>);
> +  static_assert(!__closure_has_simple_call_op<decltype(views::filter(std::declval<F>()))>);
> +  static_assert(!__closure_has_simple_call_op<decltype(views::transform(std::declval<F>()))>);
> +}
> +
>  // { dg-prune-output "in requirements" }
> --
> 2.32.0.93.g670b81a890
>

Re: [PATCH] libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]

[Jonathan Wakely]

On Mon, 14 Jun 2021 at 17:36, Patrick Palka via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> The _S_has_simple_extra_args mechanism is used to simplify forwarding
> of range adaptor's extra arguments when perfect forwarding call wrapper
> semantics isn't required for correctness, on a per-adaptor basis.
> Both views::take and views::drop are flagged as such, but it turns out
> perfect forwarding semantics are needed for these adaptors in some
> contrived cases, e.g. when their extra argument is a move-only class
> that's implicitly convertible to an integral type.
>
> To fix this, we could just clear the flag for views::take/drop as with
> views::split, but that'd come at the cost of acceptable diagnostics
> for ill-formed uses of these adaptors (see PR100577).
>
> This patch instead allows adaptors to parameterize their
> _S_has_simple_extra_args flag according the types of the captured extra
> arguments, so that we could conditionally disable perfect forwarding
> semantics only when the types of the extra arguments permit it.  We
> then use this finer-grained mechanism to safely disable perfect
> forwarding semantics for views::take/drop when the extra argument is
> integer-like, rather than incorrectly always disabling it.  Similarly,
> for views::split, rather than always enabling perfect forwarding
> semantics we now safely disable it when the extra argument is a scalar
> or a view, and recover good diagnostics for these common cases.
>
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?

OK for both.


>
>         PR libstdc++/100940
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/ranges (__adaptor::_RangeAdaptor): Document the
>         template form of _S_has_simple_extra_args.
>         (__adaptor::__adaptor_has_simple_extra_args): Add _Args template
>         parameter pack.  Try to treat _S_has_simple_extra_args as a
>         variable template parameterized by _Args.
>         (__adaptor::_Partial): Pass _Arg/_Args to the constraint
>         __adaptor_has_simple_extra_args.
>         (views::_Take::_S_has_simple_extra_args): Templatize according
>         to the type of the extra argument.
>         (views::_Drop::_S_has_simple_extra_args): Likewise.
>         (views::_Split::_S_has_simple_extra_args): Define.
>         * testsuite/std/ranges/adaptors/100577.cc (test01, test02):
>         Adjust after changes to _S_has_simple_extra_args mechanism.
>         (test03): Define.
> ---
>  libstdc++-v3/include/std/ranges               | 37 +++++++++----
>  .../testsuite/std/ranges/adaptors/100577.cc   | 55 ++++++++++++-------
>  2 files changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 220a44e11a8..856975c6934 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -792,7 +792,9 @@ namespace views::__adaptor
>    //
>    // The optional static data member _Derived::_S_has_simple_extra_args should
>    // be defined to true if the behavior of this adaptor is independent of the
> -  // constness/value category of the extra arguments.
> +  // constness/value category of the extra arguments.  This data member could
> +  // also be defined as a variable template parameterized by the types of the
> +  // extra arguments.
>    template<typename _Derived>
>      struct _RangeAdaptor
>      {
> @@ -814,9 +816,10 @@ namespace views::__adaptor
>      concept __closure_has_simple_call_op = _Adaptor::_S_has_simple_call_op;
>
>    // True if the behavior of the range adaptor non-closure _Adaptor is
> -  // independent of the value category of its extra arguments.
> -  template<typename _Adaptor>
> -    concept __adaptor_has_simple_extra_args = _Adaptor::_S_has_simple_extra_args;
> +  // independent of the value category of its extra arguments _Args.
> +  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...>;
>
>    // A range adaptor closure that represents partial application of
>    // the range adaptor _Adaptor with arguments _Args.
> @@ -893,7 +896,7 @@ namespace views::__adaptor
>    // This lets us get away with a single operator() overload, which makes
>    // overload resolution failure diagnostics more concise.
>    template<typename _Adaptor, typename... _Args>
> -    requires __adaptor_has_simple_extra_args<_Adaptor>
> +    requires __adaptor_has_simple_extra_args<_Adaptor, _Args...>
>      struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure
>      {
>        tuple<_Args...> _M_args;
> @@ -922,7 +925,7 @@ namespace views::__adaptor
>    // A lightweight specialization of the above template for the common case
>    // where _Adaptor accepts a single extra argument.
>    template<typename _Adaptor, typename _Arg>
> -    requires __adaptor_has_simple_extra_args<_Adaptor>
> +    requires __adaptor_has_simple_extra_args<_Adaptor, _Arg>
>      struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure
>      {
>        _Arg _M_arg;
> @@ -2094,7 +2097,12 @@ namespace views::__adaptor
>
>        using _RangeAdaptor<_Take>::operator();
>        static constexpr int _S_arity = 2;
> -      static constexpr bool _S_has_simple_extra_args = true;
> +      // 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>;
>      };
>
>      inline constexpr _Take take;
> @@ -2334,7 +2342,9 @@ namespace views::__adaptor
>
>        using _RangeAdaptor<_Drop>::operator();
>        static constexpr int _S_arity = 2;
> -      static constexpr bool _S_has_simple_extra_args = true;
> +      template<typename _Tp>
> +       static constexpr bool _S_has_simple_extra_args
> +         = _Take::_S_has_simple_extra_args<_Tp>;
>      };
>
>      inline constexpr _Drop drop;
> @@ -3212,9 +3222,14 @@ namespace views::__adaptor
>
>        using _RangeAdaptor<_Split>::operator();
>        static constexpr int _S_arity = 2;
> -      // The second argument of views::split is _not_ simple -- it can be a
> -      // non-view range, the value category of which affects whether the call is
> -      // well-formed.  So we must not define _S_has_simple_extra_args to true.
> +      // The pattern argument of views::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
> +      // simple.
> +      template<typename _Pattern>
> +       static constexpr bool _S_has_simple_extra_args
> +         = is_scalar_v<_Pattern> || (view<_Pattern>
> +                                     && copy_constructible<_Pattern>);
>      };
>
>      inline constexpr _Split split;
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> index 29176c8b7b0..8ef084621f9 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> @@ -28,16 +28,18 @@ namespace views = std::ranges::views;
>  void
>  test01()
>  {
> -  // Verify all multi-argument adaptors except for views::split are denoted
> -  // to have simple extra arguments.
> +  // Verify adaptors are deemed to have simple extra arguments when appropriate.
>    using views::__adaptor::__adaptor_has_simple_extra_args;
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::transform)>);
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::filter)>);
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop)>);
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::take)>);
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::take_while)>);
> -  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop_while)>);
> -  static_assert(!__adaptor_has_simple_extra_args<decltype(views::split)>);
> +  using std::identity;
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::transform), identity>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::filter), identity>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop), int>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::take), int>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::take_while), identity>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop_while), identity>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::split), std::string_view>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::split), char>);
> +  static_assert(!__adaptor_has_simple_extra_args<decltype(views::split), std::string>);
>
>    // Verify all adaptor closures except for views::split(pattern) have a simple
>    // operator().
> @@ -53,15 +55,17 @@ test01()
>    __closure_has_simple_call_op auto a08 = views::common;
>    __closure_has_simple_call_op auto a09 = views::reverse;
>    __closure_has_simple_call_op auto a10 = views::keys;
> +  __closure_has_simple_call_op auto a11 = views::split(' ');
>    // Verify composition of simple closures is simple.
>    __closure_has_simple_call_op auto b
> -    = (a00 | a01) | (a02 | a03) | (a04 | a05 | a06) | (a07 | a08 | a09 | a10);
> +    = (a00 | a01) | (a02 | a03) | (a04 | a05 | a06) | (a07 | a08 | a09 | a10) | a11;
>
> -  // Verify views::split is the exception.
> -  auto a11 = views::split(' ');
> -  static_assert(!__closure_has_simple_call_op<decltype(a11)>);
> -  static_assert(!__closure_has_simple_call_op<decltype(a11 | a00)>);
> -  static_assert(!__closure_has_simple_call_op<decltype(a00 | a11)>);
> +  // Verify views::split(non_view_range) is an exception.
> +  extern std::string s;
> +  auto a12 = views::split(s);
> +  static_assert(!__closure_has_simple_call_op<decltype(a12)>);
> +  static_assert(!__closure_has_simple_call_op<decltype(a12 | a00)>);
> +  static_assert(!__closure_has_simple_call_op<decltype(a00 | a12)>);
>  }
>
>  void
> @@ -71,18 +75,14 @@ test02()
>    // fallback deleted overload, so when a call is ill-formed overload resolution
>    // fails.
>    extern int x[10];
> -  auto badarg = nullptr;
> +  struct { } badarg;
>    views::transform(badarg)(x); // { dg-error "no match" }
>    views::filter(badarg)(x); // { dg-error "no match" }
> -  views::take(badarg)(x); // { dg-error "no match" }
> -  views::drop(badarg)(x); // { dg-error "no match" }
>    views::take_while(badarg)(x); // { dg-error "no match" }
>    views::drop_while(badarg)(x); // { dg-error "no match" }
>
>    (views::transform(badarg) | views::all)(x); // { dg-error "no match" }
>    (views::filter(badarg) | views::all)(x); // { dg-error "no match" }
> -  (views::take(badarg) | views::all)(x); // { dg-error "no match" }
> -  (views::drop(badarg) | views::all)(x); // { dg-error "no match" }
>    (views::take_while(badarg) | views::all)(x); // { dg-error "no match" }
>    (views::drop_while(badarg) | views::all)(x); // { dg-error "no match" }
>
> @@ -96,6 +96,21 @@ test02()
>    a0(x); // { dg-error "no match" };
>    auto a1 = a0 | views::all;
>    a1(x); // { dg-error "no match" }
> +
> +  views::take(badarg)(x); // { dg-error "deleted" }
> +  views::drop(badarg)(x); // { dg-error "deleted" }
> +  (views::take(badarg) | views::all)(x); // { dg-error "deleted" }
> +  (views::drop(badarg) | views::all)(x); // { dg-error "deleted" }
> +}
> +
> +void
> +test03()
> +{
> +  // PR libstdc++/100940
> +  extern int x[10];
> +  struct S { operator int() && { return 5; }; };
> +  x | std::views::take(S{});
> +  x | std::views::drop(S{});
>  }
>
>  // { dg-prune-output "in requirements" }
> --
> 2.32.0.93.g670b81a890
>