From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 1E3453948D95 for ; Wed, 26 May 2021 18:52:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1E3453948D95 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-437-AqVbsIX-MTyJJX1kHRL9LA-1; Wed, 26 May 2021 14:52:23 -0400 X-MC-Unique: AqVbsIX-MTyJJX1kHRL9LA-1 Received: by mail-qk1-f200.google.com with SMTP id e8-20020a05620a2088b02903a5edeec4d6so1535388qka.11 for ; Wed, 26 May 2021 11:52:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=ptwCUhSOd2R0Jk0AxOJly5T/ZRiujDjif6VXlY9Sy/0=; b=n1ivGqlAdwXAYb7raHgzfVif5pB74dBHRsxi1k1KbA7JSByC+ShiYqN72lx1WfU+PP OvRyeApITGFcC7h6qmPbVwmhbF7+Sou1SZrF9mmk05ahYQAGX1MJxx4orbgATjL2uhQB 05l8W5F7eIM4TrOpwIHpN+dLZifraM3SpMHpCGx7km9vsOtg0Zm/6g2k9T0jk4IE0V03 I2MsC4QTibehjomX6axGETeSI9MT7Qtq1WksRqS0kq8cLwWCBmjQvMeioNoT8Sk/8O5O 9walxEnwdKD6pHlOjVNcJkPkao9gKP0Zst+1oTVhZBdcoy+sX2RdG2ssmm3RniZ6xVdn jT9w== X-Gm-Message-State: AOAM532+u4JO2eelExgc2gtI8VGbE/GGNCJabkttbcqhagpsmbrTvzm3 ahBnPJLsYCBdm6DD4rkjtei7c1UFgBGs+8ko3FSee9slB5O1QY8tY8uFV5D/+/lArAOEM7nPglg IGNOyPF0W4zhiRdQ= X-Received: by 2002:a05:620a:1252:: with SMTP id a18mr41685418qkl.416.1622055143075; Wed, 26 May 2021 11:52:23 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw4rHnTv/DfAyptV+k+pMCePWnJTPK8ytVmOrmX5sX/uaIBGwgpO5wX5DzRHpwjr7CATBeGcg== X-Received: by 2002:a05:620a:1252:: with SMTP id a18mr41685381qkl.416.1622055142557; Wed, 26 May 2021 11:52:22 -0700 (PDT) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id n13sm1983646qtl.48.2021.05.26.11.52.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 May 2021 11:52:22 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Wed, 26 May 2021 14:52:21 -0400 (EDT) To: Patrick Palka cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: Re: [PATCH] libstdc++: Simplify range adaptors' forwarding of bound args when possible In-Reply-To: <20210514182749.602087-1-ppalka@redhat.com> Message-ID: <5adc399e-2efe-988f-dd6a-7d95a5f1f1a8@idea> References: <20210514182749.602087-1-ppalka@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-16.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, URI_HEX autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 May 2021 18:52:34 -0000 On Fri, 14 May 2021, Patrick Palka wrote: > r11-8053 rewrote the range adaptor implementation in conformance with > P2281, 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 wg21.link/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 of P2281 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. Ping. > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11? > > 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. > --- > libstdc++-v3/include/std/ranges | 119 +++++++++++++++++- > .../testsuite/std/ranges/adaptors/100577.cc | 101 +++++++++++++++ > 2 files changed, 219 insertions(+), 1 deletion(-) > create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc > > diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges > index 48100e9d7f2..0f69d4f0839 100644 > --- a/libstdc++-v3/include/std/ranges > +++ b/libstdc++-v3/include/std/ranges > @@ -756,6 +756,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). > @@ -781,6 +785,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 > { > @@ -795,6 +803,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 > @@ -808,7 +827,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 > @@ -865,6 +884,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>())); }; > @@ -900,6 +972,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> > @@ -981,6 +1079,8 @@ namespace views::__adaptor > else > return subrange{std::forward<_Range>(__r)}; > } > + > + static constexpr bool _S_has_simple_call_op = true; > }; > > inline constexpr _All all; > @@ -1353,6 +1453,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; > @@ -1727,6 +1828,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; > @@ -1907,6 +2009,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; > @@ -2026,6 +2129,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; > @@ -2145,6 +2249,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; > @@ -2228,6 +2333,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; > @@ -2645,6 +2751,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; > @@ -3078,6 +3186,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; > @@ -3217,6 +3328,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; > @@ -3346,6 +3459,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; > @@ -3723,6 +3838,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..719ba15d947 > --- /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(foo) 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" } > -- > 2.31.1.606.gdf6c4f722c > >