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

[Jonathan Wakely <jwakely@redhat.com>]

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
>