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

Message ID 20210615185321.1082491-1-ppalka@redhat.com
State New
Headers show
Series
  • libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]
Related show

Commit Message

Jakub Jelinek via Gcc-patches June 15, 2021, 6:53 p.m.
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(-)

-- 
2.32.0.93.g670b81a890

Comments

Jakub Jelinek via Gcc-patches June 15, 2021, 7:28 p.m. | #1
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
Jakub Jelinek via Gcc-patches June 16, 2021, 1:43 p.m. | #2
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

>

Patch

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" }