From patchwork Fri May 14 18:27:49 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Palka X-Patchwork-Id: 1478640 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=dHw6Ew16; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4FhcTn6hDkz9sRN for ; Sat, 15 May 2021 04:28:12 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0AC7D3944821; Fri, 14 May 2021 18:28:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0AC7D3944821 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1621016887; bh=qqwEjOBjxhReh5W52r+e1RX18TMCztJ8Fy1X6njr9+o=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=dHw6Ew16arHofUPiRugvP8SuWIxQJ5SYadGsJkoY5J9o+p79owwegsmKFk/iq65jv 2+nriVMRvE+RAIXrET7nzqsB2mqhBRAe09KH8Cht6pnhVv+pXzwT1pKRWKvqX4A3iO TrVSNdt1tizvFYLBQBGzrswcaqZtOeCxw4t+KSis= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id CC050393D000 for ; Fri, 14 May 2021 18:28:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CC050393D000 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-602-QTND-AX7OpCz-3t11DNuaQ-1; Fri, 14 May 2021 14:27:56 -0400 X-MC-Unique: QTND-AX7OpCz-3t11DNuaQ-1 Received: by mail-qk1-f199.google.com with SMTP id p133-20020a37428b0000b02902de31dd8da3so22518139qka.1 for ; Fri, 14 May 2021 11:27:56 -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:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=qqwEjOBjxhReh5W52r+e1RX18TMCztJ8Fy1X6njr9+o=; b=ZKwCnv6S2zSSy0oqVTWQx+cfyi8wo0/6hvHUGJM3W+Rzwj3EYDoZsxE+U4GjCJT1MW aj2s0ZnAKdE6j5W4QaMRaDaMRB1z4jy26ZblrrEYJKJj3BDZXohLgRxUDm//bWcFKorH /oCGTaRwQDeIVZNNoNaiDzkT9uAR0008kHPefwqrjHUNvHeU1vcNtuUXVwMU7d4fdXxz LFTCVtkURyaJKJsaerap9Y/JW1GHlYK4TzmhN79qZW9hFSgMkbVx0vAlIDVOu8WUdPCF Iuila+1oZRsv2MyevBmpD6NFx06HaUAgVflLytF/6MOV99m7wFuRxTKfHc/gUV8TOSPF VdGA== X-Gm-Message-State: AOAM530dZuZINVIDaBGNwzfMhUGYKgfN8QvsUVowXMvdUwBivsTJ1h5O 4OSvvY373ssjRYcsGnFUSJNDE+Y6HkpAz4GiR7vRAtbaIP//X20bXeW0Obzf4N2M24gPxoAK7Hs E8yApoMgTp+qdjD+mTLDR+ItDYRSqAQqOdVIAUj5Gmr7aCZxB8+8aj45FkKu03985Gg0= X-Received: by 2002:a0c:ef42:: with SMTP id t2mr44311928qvs.48.1621016874989; Fri, 14 May 2021 11:27:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw5KThCYwb/44KF/ZnTdGAq2WJH3IWRREilSrVYykFY4oIU0QeMgDTiVTflBAExzHCU9tF8XQ== X-Received: by 2002:a0c:ef42:: with SMTP id t2mr44311885qvs.48.1621016874362; Fri, 14 May 2021 11:27:54 -0700 (PDT) Received: from localhost.localdomain (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id p10sm5241538qkg.74.2021.05.14.11.27.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 May 2021 11:27:53 -0700 (PDT) To: gcc-patches@gcc.gnu.org Subject: [PATCH] libstdc++: Simplify range adaptors' forwarding of bound args when possible Date: Fri, 14 May 2021 14:27:49 -0400 Message-Id: <20210514182749.602087-1-ppalka@redhat.com> X-Mailer: git-send-email 2.31.1.606.gdf6c4f722c MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-16.3 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Patrick Palka via Gcc-patches From: Patrick Palka Reply-To: Patrick Palka Cc: libstdc++@gcc.gnu.org Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" 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. 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" }