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 [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 086ED3858D38 for ; Mon, 17 Apr 2023 03:24:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 086ED3858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1681701868; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4AFWtGLYSOr/DSA7Rl70Djd+4BRHJsRwW9Y57Evqt2o=; b=hzcj3BapTpB+EYfQN8QTFb5B31+DTwUW6fAV8EluhjfXp3R/VGotmaZbFpRYgcD/jR7aBp qeNp+PHw0U2FNKT7kK73eDQNlf/nk655mNQKpSMww8sqsG6l9blPUJzxXH4AeapqvcZPBl ox+nE7kjwusIkmYmekXmRSIJsrsZduc= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-88-qvzm3ZMFOh61klfUyYLHoQ-1; Sun, 16 Apr 2023 23:24:27 -0400 X-MC-Unique: qvzm3ZMFOh61klfUyYLHoQ-1 Received: by mail-qv1-f71.google.com with SMTP id h6-20020a0cd806000000b005a33510e95aso10663499qvj.16 for ; Sun, 16 Apr 2023 20:24:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681701867; x=1684293867; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=4AFWtGLYSOr/DSA7Rl70Djd+4BRHJsRwW9Y57Evqt2o=; b=ClLOoNh9Gp1R4WHgAug4f9n9TX3xqcJtdkQqywYHzc0K4K1ky2jTnEPq812GIp6O/i OASLfiWdPYL4jb9ws6mGDOsr9UKMLIA/Eg7PfIKMYWo9gYgQ3sFBvYmvElRJdvlGRDl6 aQ3pmD9fNBJMlZsaU7eRe3Fr5mwCy9xc25/aDugu3AC4k+6fLNugKWO81AK8ix9r1Qen H35Aipm6HOMzfHdVnZwN4lxSBZzX4CVAeAZSFxNW2GjNUSv/qGPN8iey1bMMZtdzpdsX K8OPWIAQaZtw/rkySTv09c4EgYrUODrGYSD2LKHMrFb4IGctUmh6JWg8fRpqKsLYSHuK AX/A== X-Gm-Message-State: AAQBX9doZHsee5Blu8Tm6ZTNgEW9+a79zR2bcWHeD+dbQfMvvClJ49s8 SFOwn/TYzeysrCVvBpVqOC33EkHMNuTofsn8eDFqbK+r2iOXrGcN7GQzHit8YzrJPtJT39DAfET gwLx5rLBaSgF2HmM= X-Received: by 2002:a05:6214:4108:b0:5ac:96c3:14d4 with SMTP id kc8-20020a056214410800b005ac96c314d4mr11379277qvb.17.1681701867038; Sun, 16 Apr 2023 20:24:27 -0700 (PDT) X-Google-Smtp-Source: AKy350YuBRYcxKzzkWT/AQiWYmLhK3vw2sK6xr0lbsWLnC5Gy5MR0nBsNpqPDt0duhdfQ6aKGXiqFw== X-Received: by 2002:a05:6214:4108:b0:5ac:96c3:14d4 with SMTP id kc8-20020a056214410800b005ac96c314d4mr11379269qvb.17.1681701866645; Sun, 16 Apr 2023 20:24:26 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id o5-20020a0cc385000000b005ef4ee791casm2736025qvi.120.2023.04.16.20.24.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 16 Apr 2023 20:24:26 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Sun, 16 Apr 2023 23:24:25 -0400 (EDT) To: Patrick Palka cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: Re: [PATCH 1/2] libstdc++: Convert _RangeAdaptorClosure into a CRTP class [PR108827] In-Reply-To: <20230414194512.2569383-1-ppalka@redhat.com> Message-ID: References: <20230414194512.2569383-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=-13.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 14 Apr 2023, Patrick Palka wrote: > Using the CRTP idiom for this base class avoids bloating the size of a > pipeline when adding distinct empty range adaptor closure objects to it, > as detailed in section 4.1 of P2387R3. > > But it means we can no longer define its operator| overloads as hidden > friends, since each instantiation of _RangeAdaptorClosure would then > introduce its own logically different hidden friends. So for example > during overload resolution for the outer pipe operator in > > :x | (views::reverse | views::join) > > we'd have to consider 6 different hidden operator| friends: > > 2 from _RangeAdaptorClosure<_Reverse> > 2 from _RangeAdaptorClosure<_Join> > 2 from _RangeAdaptorClosure<_Pipe<_Reverse, _Join>> > > which is wasteful and can even cause hard errors in some cases. So we > instead define the operator| overloads at namespace scope in an isolated > namespace. On second thought, since this doesn't fix a bug or add new functionality it seems more like GCC 14 material. The size reduction is nice but it's probably not a big deal in practice since adaptor pipelines are usually very transient objects that don't get passed around as function arguments etc. But perhaps the second patch implementing range_adaptor_closure would be desirable for GCC 13? I'll post an updated standalone version of that patch for separate consideration. > > PR libstdc++/108827 > > libstdc++-v3/ChangeLog: > > * include/std/ranges (__adaptor::_RangeAdaptorClosure): Move ... > (__adaptor::__closure::_RangeAdaptorClosure): ... here and turn > it into a CRTP class template. Move hidden operator| friends > into namespace scope and adjust their constraints. Add a > using-declaration for this at __adaptor::_RangeAdaptorClosure. > (__closure::__is_range_adaptor_closure_fn): Define. > (__closure::__is_range_adaptor_closure): Define. > (__adaptor::_Partial): Adjust use of _RangeAdaptorClosure. > (__adaptor::_Pipe): Likewise. > (views::_All): Likewise. > (views::_Join): Likewise. > (views::_Common): Likewise. > (views::_Reverse): Likewise. > (views::_Elements): Likewise. > (views::_Adjacent): Likewise. > (views::_AsRvalue): Likewise. > (views::_Enumerate): Likewise. > (views::_AsConst): Likewise. > * testsuite/std/ranges/adaptors/all.cc: Reintroduce > static_assert expecting that adding empty range adaptor > closure objects to a pipeline doesn't increase the size of a > pipeline. > --- > libstdc++-v3/include/std/ranges | 69 +++++++++++-------- > .../testsuite/std/ranges/adaptors/all.cc | 7 -- > 2 files changed, 42 insertions(+), 34 deletions(-) > > diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges > index 283d757faa4..531ec6f68b3 100644 > --- a/libstdc++-v3/include/std/ranges > +++ b/libstdc++-v3/include/std/ranges > @@ -872,30 +872,45 @@ namespace views::__adaptor > template > 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 > + namespace __closure > { > + // 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. > + template > + struct _RangeAdaptorClosure > + { }; > + > + template > + requires (!same_as<_Tp, _RangeAdaptorClosure<_Up>>) > + void __is_range_adaptor_closure_fn > + (const _Tp&, const _RangeAdaptorClosure<_Up>&); // not defined > + > + template > + concept __is_range_adaptor_closure > + = requires (_Tp __t) { __closure::__is_range_adaptor_closure_fn(__t, __t); }; > + > // range | adaptor is equivalent to adaptor(range). > template > - requires derived_from, _RangeAdaptorClosure> > + requires __is_range_adaptor_closure<_Self> > && __adaptor_invocable<_Self, _Range> > - friend constexpr auto > + constexpr auto > operator|(_Range&& __r, _Self&& __self) > { return std::forward<_Self>(__self)(std::forward<_Range>(__r)); } > > // Compose the adaptors __lhs and __rhs into a pipeline, returning > // another range adaptor closure object. > template > - requires derived_from<_Lhs, _RangeAdaptorClosure> > - && derived_from<_Rhs, _RangeAdaptorClosure> > - friend constexpr auto > + requires __is_range_adaptor_closure<_Lhs> > + && __is_range_adaptor_closure<_Rhs> > + constexpr auto > operator|(_Lhs __lhs, _Rhs __rhs) > { return _Pipe<_Lhs, _Rhs>{std::move(__lhs), std::move(__rhs)}; } > - }; > + } // namespace __closure > + > + using __closure::_RangeAdaptorClosure; > > // The base class of every range adaptor non-closure. > // > @@ -937,7 +952,7 @@ namespace views::__adaptor > // A range adaptor closure that represents partial application of > // the range adaptor _Adaptor with arguments _Args. > template > - struct _Partial : _RangeAdaptorClosure > + struct _Partial : _RangeAdaptorClosure<_Partial<_Adaptor, _Args...>> > { > tuple<_Args...> _M_args; > > @@ -978,7 +993,7 @@ namespace views::__adaptor > // A lightweight specialization of the above primary template for > // the common case where _Adaptor accepts a single extra argument. > template > - struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure > + struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure<_Partial<_Adaptor, _Arg>> > { > _Arg _M_arg; > > @@ -1011,7 +1026,7 @@ namespace views::__adaptor > template > requires __adaptor_has_simple_extra_args<_Adaptor, _Args...> > && (is_trivially_copyable_v<_Args> && ...) > - struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure > + struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure<_Partial<_Adaptor, _Args...>> > { > tuple<_Args...> _M_args; > > @@ -1041,7 +1056,7 @@ namespace views::__adaptor > template > requires __adaptor_has_simple_extra_args<_Adaptor, _Arg> > && is_trivially_copyable_v<_Arg> > - struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure > + struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure<_Partial<_Adaptor, _Arg>> > { > _Arg _M_arg; > > @@ -1066,7 +1081,7 @@ namespace views::__adaptor > // A range adaptor closure that represents composition of the range > // adaptor closures _Lhs and _Rhs. > template > - struct _Pipe : _RangeAdaptorClosure > + struct _Pipe : _RangeAdaptorClosure<_Pipe<_Lhs, _Rhs>> > { > [[no_unique_address]] _Lhs _M_lhs; > [[no_unique_address]] _Rhs _M_rhs; > @@ -1102,7 +1117,7 @@ namespace views::__adaptor > template > requires __closure_has_simple_call_op<_Lhs> > && __closure_has_simple_call_op<_Rhs> > - struct _Pipe<_Lhs, _Rhs> : _RangeAdaptorClosure > + struct _Pipe<_Lhs, _Rhs> : _RangeAdaptorClosure<_Pipe<_Lhs, _Rhs>> > { > [[no_unique_address]] _Lhs _M_lhs; > [[no_unique_address]] _Rhs _M_rhs; > @@ -1264,7 +1279,7 @@ namespace views::__adaptor > concept __can_owning_view = requires { owning_view{std::declval<_Range>()}; }; > } // namespace __detail > > - struct _All : __adaptor::_RangeAdaptorClosure > + struct _All : __adaptor::_RangeAdaptorClosure<_All> > { > template > static constexpr bool > @@ -3048,7 +3063,7 @@ namespace views::__adaptor > = requires { join_view>{std::declval<_Range>()}; }; > } // namespace __detail > > - struct _Join : __adaptor::_RangeAdaptorClosure > + struct _Join : __adaptor::_RangeAdaptorClosure<_Join> > { > template > requires __detail::__can_join_view<_Range> > @@ -3842,7 +3857,7 @@ namespace views::__adaptor > = requires { common_view{std::declval<_Range>()}; }; > } // namespace __detail > > - struct _Common : __adaptor::_RangeAdaptorClosure > + struct _Common : __adaptor::_RangeAdaptorClosure<_Common> > { > template > requires __detail::__already_common<_Range> > @@ -3963,7 +3978,7 @@ namespace views::__adaptor > = requires { reverse_view{std::declval<_Range>()}; }; > } // namespace __detail > > - struct _Reverse : __adaptor::_RangeAdaptorClosure > + struct _Reverse : __adaptor::_RangeAdaptorClosure<_Reverse> > { > template > requires __detail::__is_reverse_view> > @@ -4363,7 +4378,7 @@ namespace views::__adaptor > } // namespace __detail > > template > - struct _Elements : __adaptor::_RangeAdaptorClosure > + struct _Elements : __adaptor::_RangeAdaptorClosure<_Elements<_Nm>> > { > template > requires __detail::__can_elements_view<_Nm, _Range> > @@ -5484,7 +5499,7 @@ namespace views::__adaptor > } > > template > - struct _Adjacent : __adaptor::_RangeAdaptorClosure > + struct _Adjacent : __adaptor::_RangeAdaptorClosure<_Adjacent<_Nm>> > { > template > requires (_Nm == 0) || __detail::__can_adjacent_view<_Nm, _Range> > @@ -8609,7 +8624,7 @@ namespace views::__adaptor > concept __can_as_rvalue_view = requires { as_rvalue_view(std::declval<_Tp>()); }; > } > > - struct _AsRvalue : __adaptor::_RangeAdaptorClosure > + struct _AsRvalue : __adaptor::_RangeAdaptorClosure<_AsRvalue> > { > template > requires __detail::__can_as_rvalue_view<_Range> > @@ -8918,7 +8933,7 @@ namespace views::__adaptor > = requires { enumerate_view>(std::declval<_Tp>()); }; > } > > - struct _Enumerate : __adaptor::_RangeAdaptorClosure > + struct _Enumerate : __adaptor::_RangeAdaptorClosure<_Enumerate> > { > template > requires __detail::__can_enumerate_view<_Range> > @@ -9004,7 +9019,7 @@ namespace views::__adaptor > concept __can_as_const_view = requires { as_const_view(std::declval<_Range>()); }; > } > > - struct _AsConst : __adaptor::_RangeAdaptorClosure > + struct _AsConst : __adaptor::_RangeAdaptorClosure<_AsConst> > { > template > constexpr auto > diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > index 57cb542174b..6bbaf73a20a 100644 > --- a/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/all.cc > @@ -105,12 +105,6 @@ static_assert(std::is_empty_v | views::common > | views::keys > | views::reverse)>); > -#if 0 > -// Adding empty range adaptor closure objects to a pipeline used to not > -// increase the size of the pipeline, but now that our range adaptor closure > -// objects derive from a common empty base class, [[no_unique_address]] can no > -// longer make two empty adjacent range adaptor closure objects occupy the same > -// data member address. > static_assert(sizeof(decltype(views::take(5) | views::drop(5))) > == sizeof(decltype(views::take(5) > | views::join > @@ -119,7 +113,6 @@ static_assert(sizeof(decltype(views::take(5) | views::drop(5))) > | views::keys > | views::drop(5) > | views::reverse))); > -#endif > > template > void > -- > 2.40.0.335.g9857273be0 > >