Re: [PATCH 1/2] libstdc++: Convert _RangeAdaptorClosure into a CRTP class [PR108827]
On Wed, 16 Aug 2023 at 17:06, Patrick Palka via Libstdc++
wrote:
>
> On Sun, Apr 16, 2023 at 11:24 PM Patrick Palka wrote:
> >
> > 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.
>
> Ping, does this look OK for trunk?
OK for trunk, thanks.
>
> >
> > 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
> > >
Re: [PATCH 1/2] libstdc++: Convert _RangeAdaptorClosure into a CRTP class [PR108827]
On Sun, Apr 16, 2023 at 11:24 PM Patrick Palka wrote:
>
> 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.
Ping, does this look OK for trunk?
>
> 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
> > - require
Re: [PATCH 1/2] libstdc++: Convert _RangeAdaptorClosure into a CRTP class [PR108827]
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 _Pip
[PATCH 1/2] libstdc++: Convert _RangeAdaptorClosure into a CRTP class [PR108827]
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.
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
