| Message ID | 20210614163543.502297-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940] | expand |
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 >
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" }