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

[Jonathan Wakely <jwakely@redhat.com>]

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
>