On Mon, 14 Jun 2021 at 17:36, Patrick Palka via Libstdc++
<[email protected]> 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
>
