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