From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1888) id 7E6EE3B8A415; Thu, 10 Jun 2021 21:29:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7E6EE3B8A415 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Patrick Palka To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r11-8547] libstdc++: Simplify range adaptors' forwarding of bound args [PR100577] X-Act-Checkin: gcc X-Git-Author: Patrick Palka X-Git-Refname: refs/heads/releases/gcc-11 X-Git-Oldrev: a35c5a5fc3eb610b77833855048163486c2fdb49 X-Git-Newrev: 80495610eb8c3b1395073cebd26a079d81b73403 Message-Id: <20210610212946.7E6EE3B8A415@sourceware.org> Date: Thu, 10 Jun 2021 21:29:46 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jun 2021 21:29:46 -0000 https://gcc.gnu.org/g:80495610eb8c3b1395073cebd26a079d81b73403 commit r11-8547-g80495610eb8c3b1395073cebd26a079d81b73403 Author: Patrick Palka Date: Thu Jun 3 09:49:30 2021 -0400 libstdc++: Simplify range adaptors' forwarding of bound args [PR100577] r11-8053 rewrote the range adaptor implementation in conformance with P2281R1, making partial application act like a SFINAE-friendly perfect forwarding call wrapper. Making SFINAE-friendliness coexist with perfect forwarding here requires adding fallback deleted operator() overloads (as described in e.g. section 5.5 of P0847R6). But unfortunately, as reported in PR100577, this necessary technique of using of deleted overloads regresses diagnostics for ill-formed calls to partially applied range adaptors in GCC. Although GCC's diagnostics can arguably be improved here by having the compiler explain why the other candidates weren't viable when overload resolution selects a deleted candidate, we can also largely work around this on the library side (and achieve more concise diagnostics than by a frontend-side improvement alone) if we take advantage of the observation that not all range adaptors need perfect forwarding call wrapper semantics, in fact only views::split currently needs it, because all other range adaptors either take no extra arguments or only arguments that are expected to be freely/cheaply copyable, e.g. function objects and integer-like types. (The discussion section in P2281R1 goes into detail about why views::split is special.) To that end, this introduces opt-in flags for denoting a range adaptor as having "simple" extra arguments (in the case of a range adaptor non-closure) or having a "simple" call operator (in the case of a range adaptor closure). These flags are then used to conditionally simplify the operator() for the generic _Partial and _Pipe class templates, down from needing three overloads thereof (including one defined as deleted) to just needing a single overload. The end result is that diagnostic quality is restored for all adaptors except for views::split, and diagnostics for the adaptors are generally made more concise since there's only a single _Partial/_Pipe overload to diagnose instead of three of them. libstdc++-v3/ChangeLog: PR libstdc++/100577 * include/std/ranges (_RangeAdaptorClosure): Document _S_has_simple_call_op mechanism. (_RangeAdaptor): Document _S_has_simple_extra_args mechanism. (__closure_has_simple_call_op): New concept. (__adaptor_has_simple_extra_args): Likewise. (_Partial<_Adaptor, _Args...>): New partial specialization. (_Partial<_Adaptor, _Arg>): Likewise. (_Pipe<_Lhs, _Rhs>): Likewise. (views::_All::_S_has_simple_call_op): Define to true. (views::_Filter::_S_has_simple_extra_args): Likewise. (views::_Transform::_S_has_simple_extra_args): Likewise. (views::_Take::_S_has_simple_extra_args): Likewise. (views::_TakeWhile::_S_has_simple_extra_args): Likewise. (views::_Drop::_S_has_simple_extra_args): Likewise. (views::_DropWhile::_S_has_simple_extra_args): Likewise. (views::_Join::_S_has_simple_call_op): Likewise. (views::_Split): Document why we don't define _S_has_simple_extra_args to true for this adaptor. (views::_Common::_S_has_simple_call_op): Define to true. (views::_Reverse::_S_has_simple_call_op): Likewise. (views::_Elements::_S_has_simple_call_op): Likewise. * testsuite/std/ranges/adaptors/100577.cc: New test. (cherry picked from commit 57ed620ebfa4275d04efeec973e88f506b0e3ba8) Diff: --- libstdc++-v3/include/std/ranges | 119 ++++++++++++++++++++- .../testsuite/std/ranges/adaptors/100577.cc | 101 +++++++++++++++++ 2 files changed, 219 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges index 9ba28afbfce..b3173f4f7c7 100644 --- a/libstdc++-v3/include/std/ranges +++ b/libstdc++-v3/include/std/ranges @@ -760,6 +760,10 @@ namespace views::__adaptor struct _Pipe; // The base class of every range adaptor closure. + // + // The derived class should define the optional static data member + // _S_has_simple_call_op to true if the behavior of this adaptor is + // independent of the constness/value category of the adaptor object. struct _RangeAdaptorClosure { // range | adaptor is equivalent to adaptor(range). @@ -785,6 +789,10 @@ namespace views::__adaptor // The static data member _Derived::_S_arity must contain the total number of // arguments that the adaptor takes, and the class _Derived must introduce // _RangeAdaptor::operator() into the class scope via a using-declaration. + // + // 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. template struct _RangeAdaptor { @@ -799,6 +807,17 @@ namespace views::__adaptor } }; + // True if the range adaptor closure _Adaptor has a simple operator(), i.e. + // one that's not overloaded according to constness or value category of the + // _Adaptor object. + template + 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 + concept __adaptor_has_simple_extra_args = _Adaptor::_S_has_simple_extra_args; + // A range adaptor closure that represents partial application of // the range adaptor _Adaptor with arguments _Args. template @@ -812,7 +831,7 @@ namespace views::__adaptor { } // Invoke _Adaptor with arguments __r, _M_args... according to the - // value category of the range adaptor closure object. + // value category of this _Partial object. template requires __adaptor_invocable<_Adaptor, _Range, const _Args&...> constexpr auto @@ -869,6 +888,59 @@ namespace views::__adaptor operator()(_Range&& __r) const && = delete; }; + // 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. + template + requires __adaptor_has_simple_extra_args<_Adaptor> + struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure + { + tuple<_Args...> _M_args; + + constexpr + _Partial(_Args... __args) + : _M_args(std::move(__args)...) + { } + + // Invoke _Adaptor with arguments __r, const _M_args&... regardless + // of the value category of this _Partial object. + template + requires __adaptor_invocable<_Adaptor, _Range, const _Args&...> + constexpr auto + operator()(_Range&& __r) const + { + auto __forwarder = [&__r] (const auto&... __args) { + return _Adaptor{}(std::forward<_Range>(__r), __args...); + }; + return std::apply(__forwarder, _M_args); + } + + static constexpr bool _S_has_simple_call_op = true; + }; + + // A lightweight specialization of the above template for the common case + // where _Adaptor accepts a single extra argument. + template + requires __adaptor_has_simple_extra_args<_Adaptor> + struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure + { + _Arg _M_arg; + + constexpr + _Partial(_Arg __arg) + : _M_arg(std::move(__arg)) + { } + + template + requires __adaptor_invocable<_Adaptor, _Range, const _Arg&> + constexpr auto + operator()(_Range&& __r) const + { return _Adaptor{}(std::forward<_Range>(__r), _M_arg); } + + static constexpr bool _S_has_simple_call_op = true; + }; + template concept __pipe_invocable = requires { std::declval<_Rhs>()(std::declval<_Lhs>()(std::declval<_Range>())); }; @@ -904,6 +976,32 @@ namespace views::__adaptor constexpr auto operator()(_Range&& __r) const && = delete; }; + + // A partial specialization of the above primary template for the case where + // both adaptor operands have a simple operator(). This in turn lets us + // implement composition using a single simple operator(), which makes + // overload resolution failure diagnostics more concise. + template + requires __closure_has_simple_call_op<_Lhs> + && __closure_has_simple_call_op<_Rhs> + struct _Pipe<_Lhs, _Rhs> : _RangeAdaptorClosure + { + [[no_unique_address]] _Lhs _M_lhs; + [[no_unique_address]] _Rhs _M_rhs; + + constexpr + _Pipe(_Lhs __lhs, _Rhs __rhs) + : _M_lhs(std::move(__lhs)), _M_rhs(std::move(__rhs)) + { } + + template + requires __pipe_invocable + constexpr auto + operator()(_Range&& __r) const + { return _M_rhs(_M_lhs(std::forward<_Range>(__r))); } + + static constexpr bool _S_has_simple_call_op = true; + }; } // namespace views::__adaptor template requires is_object_v<_Range> @@ -985,6 +1083,8 @@ namespace views::__adaptor else return subrange{std::forward<_Range>(__r)}; } + + static constexpr bool _S_has_simple_call_op = true; }; inline constexpr _All all; @@ -1438,6 +1538,7 @@ namespace views::__adaptor using _RangeAdaptor<_Filter>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _Filter filter; @@ -1812,6 +1913,7 @@ namespace views::__adaptor using _RangeAdaptor<_Transform>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _Transform transform; @@ -1992,6 +2094,7 @@ namespace views::__adaptor using _RangeAdaptor<_Take>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _Take take; @@ -2111,6 +2214,7 @@ namespace views::__adaptor using _RangeAdaptor<_TakeWhile>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _TakeWhile take_while; @@ -2230,6 +2334,7 @@ namespace views::__adaptor using _RangeAdaptor<_Drop>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _Drop drop; @@ -2313,6 +2418,7 @@ namespace views::__adaptor using _RangeAdaptor<_DropWhile>::operator(); static constexpr int _S_arity = 2; + static constexpr bool _S_has_simple_extra_args = true; }; inline constexpr _DropWhile drop_while; @@ -2670,6 +2776,8 @@ namespace views::__adaptor // 3474. Nesting join_views is broken because of CTAD return join_view>{std::forward<_Range>(__r)}; } + + static constexpr bool _S_has_simple_call_op = true; }; inline constexpr _Join join; @@ -3103,6 +3211,9 @@ 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. }; inline constexpr _Split split; @@ -3242,6 +3353,8 @@ namespace views::__adaptor else return common_view{std::forward<_Range>(__r)}; } + + static constexpr bool _S_has_simple_call_op = true; }; inline constexpr _Common common; @@ -3371,6 +3484,8 @@ namespace views::__adaptor else return reverse_view{std::forward<_Range>(__r)}; } + + static constexpr bool _S_has_simple_call_op = true; }; inline constexpr _Reverse reverse; @@ -3754,6 +3869,8 @@ namespace views::__adaptor { return elements_view, _Nm>{std::forward<_Range>(__r)}; } + + static constexpr bool _S_has_simple_call_op = true; }; template diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc new file mode 100644 index 00000000000..29176c8b7b0 --- /dev/null +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc @@ -0,0 +1,101 @@ +// Copyright (C) 2021 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a } } + +// PR libstdc++/100577 + +#include + +namespace ranges = std::ranges; +namespace views = std::ranges::views; + +void +test01() +{ + // Verify all multi-argument adaptors except for views::split are denoted + // to have simple extra arguments. + using views::__adaptor::__adaptor_has_simple_extra_args; + static_assert(__adaptor_has_simple_extra_args); + static_assert(__adaptor_has_simple_extra_args); + static_assert(__adaptor_has_simple_extra_args); + static_assert(__adaptor_has_simple_extra_args); + static_assert(__adaptor_has_simple_extra_args); + static_assert(__adaptor_has_simple_extra_args); + static_assert(!__adaptor_has_simple_extra_args); + + // Verify all adaptor closures except for views::split(pattern) have a simple + // operator(). + using views::__adaptor::__closure_has_simple_call_op; + __closure_has_simple_call_op auto a00 = views::all; + __closure_has_simple_call_op auto a01 = views::transform(std::identity{}); + __closure_has_simple_call_op auto a02 = views::filter(std::identity{}); + __closure_has_simple_call_op auto a03 = views::drop(42); + __closure_has_simple_call_op auto a04 = views::take(42); + __closure_has_simple_call_op auto a05 = views::take_while(std::identity{}); + __closure_has_simple_call_op auto a06 = views::drop_while(std::identity{}); + __closure_has_simple_call_op auto a07 = views::join; + __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; + // 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); + + // Verify views::split is the exception. + auto a11 = views::split(' '); + static_assert(!__closure_has_simple_call_op); + static_assert(!__closure_has_simple_call_op); + static_assert(!__closure_has_simple_call_op); +} + +void +test02() +{ + // Range adaptor closures with a simple operator() aren't implemented using a + // fallback deleted overload, so when a call is ill-formed overload resolution + // fails. + extern int x[10]; + auto badarg = nullptr; + 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" } + + // In practice, range adaptor closures with non-simple operator() are + // implemented using a fallback deleted overload, so when a call is + // ill-formed overload resolution succeeds but selects the deleted overload + // (but only when the closure is invoked as an rvalue). + views::split(badarg)(x); // { dg-error "deleted function" } + (views::split(badarg) | views::all)(x); // { dg-error "deleted function" } + auto a0 = views::split(badarg); + a0(x); // { dg-error "no match" }; + auto a1 = a0 | views::all; + a1(x); // { dg-error "no match" } +} + +// { dg-prune-output "in requirements" }