On Wed, Sep 3, 2025 at 11:37 PM Patrick Palka <ppa...@redhat.com> wrote:
>
> On Wed, 3 Sep 2025, Tomasz Kamiński wrote:
>
> > This patch refactors ranges::_Partial to be implemented using
> _Bind_back_t.
> > This allows it to benefit from the changes in r16-3398-g250dd5b5604fbc,
> > specifically making the closure trivially copyable. Since _Bind_back_t
> > already provides an optimized implementation for a single bound argument,
> > specializations for _Partial with a single argument are now removed.
> >
> > We still preserve a specialization of _Partial for trivially
> copy-constructible
> > arguments that define only a const overload of operator(). To avoid
> > re-checking invocability constraints, this specialization calls the
> now-public,
> > unconstrained _Binder::_S_call static method instead of the constrained
> > _Binder::operator().
> >
> > The primary specialization of _Partial retains its operator(), which
> > uses a simpler __adaptor_invocable constraint that does not consider
> > member pointers, as they are not relevant here. This implementation also
> > calls _Binder::_S_call to avoid re-performing overload resolution and
> > invocability checks for _Binder::operator().
>
> Thanks for preserving/working around these _Partial optimizatinos! Note
> that
> the primary template (in C++20 mode) also just defines 3 operator()
> overloads instead of the full 8, sacrificing const&& invocability which
> presumably isn't useful in practice. (The _Pipe closure object does
> similar tricks.)
>
> The simpler __adaptor_invocable constraint was added not just because
> it's cheaper than is_invocable, but also because it made overload
> resolution failure diagnostics actually explain why the adaptor wasn't
> invocable.
Yes, the better quality diagnostic is was also an argument from preserving
the API, and using _Bind_back_t as storage.
> But now that we have builtins for is_invocable, and
> satisfaction failure replays __is_invocable failures (after Nathaniel's
> r16-2652-gbfb8615031a8c2), we might be able to get away with using
> is_invocable_v directly without sacrificing compile time performance
> and diagnostics.
I think a dedicated concept name is beneficial here.
> We could look into that as a follow-up.
>
> LGTM, the second patch in the series LGTM too.
>
> >
> > Finally, the _M_binder member (_Bind_back_t) is now marked
> > [[no_unique_address]]. This is beneficial as ranges::_Partial is used
> with
> > ranges::to, which commonly has zero or empty bound arguments (e.g.,
> stateless
> > allocators, comparators, or hash functions).
> >
> > libstdc++-v3/ChangeLog:
> >
> > * include/bits/binders.h (_Binder::_S_call): Make public.
> > * include/std/ranges (ranges::_Partial<_Adaptor, _Args...>):
> > Replace tuple<_Args...> with _Bind_back_t<_Adaptor, _Args...>.
> > (ranges::_Partial<_Adaptor, _Arg>): Remove.
> > ---
> > Tested on x86_64-linux locally. OK for trunk?
> >
> > libstdc++-v3/include/bits/binders.h | 10 +--
> > libstdc++-v3/include/std/ranges | 104 ++++------------------------
> > 2 files changed, 17 insertions(+), 97 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/binders.h
> b/libstdc++-v3/include/bits/binders.h
> > index 08a330da6fe..f274389e5b4 100644
> > --- a/libstdc++-v3/include/bits/binders.h
> > +++ b/libstdc++-v3/include/bits/binders.h
> > @@ -186,11 +186,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > void operator()(_CallArgs&&...) const && = delete;
> > #endif
> >
> > - private:
> > - using _BoundArgsStorage
> > - // _BoundArgs are required to be move-constructible, so this is
> valid.
> > - =
> decltype(__make_bound_args<_BoundArgs...>(std::declval<_BoundArgs>()...));
> > -
> > template<typename _Tp, typename... _CallArgs>
> > static constexpr
> > decltype(auto)
> > @@ -214,6 +209,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > std::forward<_CallArgs>(__call_args)...);
> > }
> >
> > + private:
> > + using _BoundArgsStorage
> > + // _BoundArgs are required to be move-constructible, so this is
> valid.
> > + =
> decltype(__make_bound_args<_BoundArgs...>(std::declval<_BoundArgs>()...));
> > +
> > [[no_unique_address]] _Fd _M_fd;
> > [[no_unique_address]] _BoundArgsStorage _M_bound_args;
> > };
> > diff --git a/libstdc++-v3/include/std/ranges
> b/libstdc++-v3/include/std/ranges
> > index 2970b2cbe00..98154a02e9e 100644
> > --- a/libstdc++-v3/include/std/ranges
> > +++ b/libstdc++-v3/include/std/ranges
> > @@ -51,6 +51,7 @@
> > #include <utility>
> > #include <variant>
> > #endif
> > +#include <bits/binders.h>
> > #include <bits/ranges_util.h>
> > #include <bits/refwrap.h>
> >
> > @@ -1047,14 +1048,15 @@ namespace views::__adaptor
> > template<typename _Adaptor, typename... _Args>
> > struct _Partial : _RangeAdaptorClosure<_Partial<_Adaptor, _Args...>>
> > {
> > - tuple<_Args...> _M_args;
> > + using _Binder = _Bind_back_t<_Adaptor, _Args...>;
> > + [[no_unique_address]] _Binder _M_binder;
> >
> > // First parameter is to ensure this constructor is never used
> > // instead of the copy/move constructor.
> > template<typename... _Ts>
> > constexpr
> > _Partial(int, _Ts&&... __args)
> > - : _M_args(std::forward<_Ts>(__args)...)
> > + : _M_binder(0, _Adaptor(), std::forward<_Ts>(__args)...)
> > { }
> >
> > // Invoke _Adaptor with arguments __r, _M_args... according to the
> > @@ -1065,75 +1067,21 @@ namespace views::__adaptor
> > constexpr auto
> > operator()(this _Self&& __self, _Range&& __r)
> > {
> > - auto __forwarder = [&__r] (auto&&... __args) {
> > - return _Adaptor{}(std::forward<_Range>(__r),
> > - std::forward<decltype(__args)>(__args)...);
> > - };
> > - return std::apply(__forwarder, __like_t<_Self,
> _Partial>(__self)._M_args);
> > + return _Binder::_S_call(__like_t<_Self,
> _Partial>(__self)._M_binder,
> > + std::forward<_Range>(__r));
> > }
> > #else
> > template<typename _Range>
> > 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);
> > - }
> > + { return _Binder::_S_call(_M_binder, std::forward<_Range>(__r)); }
> >
> > template<typename _Range>
> > requires __adaptor_invocable<_Adaptor, _Range, _Args...>
> > constexpr auto
> > operator()(_Range&& __r) &&
> > - {
> > - auto __forwarder = [&__r] (auto&... __args) {
> > - return _Adaptor{}(std::forward<_Range>(__r),
> std::move(__args)...);
> > - };
> > - return std::apply(__forwarder, _M_args);
> > - }
> > -
> > - template<typename _Range>
> > - constexpr auto
> > - operator()(_Range&& __r) const && = delete;
> > -#endif
> > - };
> > -
> > - // A lightweight specialization of the above primary template for
> > - // the common case where _Adaptor accepts a single extra argument.
> > - template<typename _Adaptor, typename _Arg>
> > - struct _Partial<_Adaptor, _Arg> :
> _RangeAdaptorClosure<_Partial<_Adaptor, _Arg>>
> > - {
> > - _Arg _M_arg;
> > -
> > - template<typename _Tp>
> > - constexpr
> > - _Partial(int, _Tp&& __arg)
> > - : _M_arg(std::forward<_Tp>(__arg))
> > - { }
> > -
> > -#if __cpp_explicit_this_parameter
> > - template<typename _Self, typename _Range>
> > - requires __adaptor_invocable<_Adaptor, _Range, __like_t<_Self,
> _Arg>>
> > - constexpr auto
> > - operator()(this _Self&& __self, _Range&& __r)
> > - {
> > - return _Adaptor{}(std::forward<_Range>(__r),
> > - __like_t<_Self, _Partial>(__self)._M_arg);
> > - }
> > -#else
> > - template<typename _Range>
> > - requires __adaptor_invocable<_Adaptor, _Range, const _Arg&>
> > - constexpr auto
> > - operator()(_Range&& __r) const &
> > - { return _Adaptor{}(std::forward<_Range>(__r), _M_arg); }
> > -
> > - template<typename _Range>
> > - requires __adaptor_invocable<_Adaptor, _Range, _Arg>
> > - constexpr auto
> > - operator()(_Range&& __r) &&
> > - { return _Adaptor{}(std::forward<_Range>(__r), std::move(_M_arg));
> }
> > + { return _Binder::_S_call(std::move(_M_binder),
> std::forward<_Range>(__r)); }
> >
> > template<typename _Range>
> > constexpr auto
> > @@ -1150,12 +1098,13 @@ namespace views::__adaptor
> > && (is_trivially_copy_constructible_v<_Args> && ...)
> > struct _Partial<_Adaptor, _Args...> :
> _RangeAdaptorClosure<_Partial<_Adaptor, _Args...>>
> > {
> > - tuple<_Args...> _M_args;
> > + using _Binder = _Bind_back_t<_Adaptor, _Args...>;
> > + [[no_unique_address]] _Binder _M_binder;
> >
> > template<typename... _Ts>
> > constexpr
> > _Partial(int, _Ts&&... __args)
> > - : _M_args(std::forward<_Ts>(__args)...)
> > + : _M_binder(0, _Adaptor(), std::forward<_Ts>(__args)...)
> > { }
> >
> > // Invoke _Adaptor with arguments __r, const _M_args&...
> regardless
> > @@ -1164,36 +1113,7 @@ namespace views::__adaptor
> > 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<typename _Adaptor, typename _Arg>
> > - requires __adaptor_has_simple_extra_args<_Adaptor, _Arg>
> > - && is_trivially_copy_constructible_v<_Arg>
> > - struct _Partial<_Adaptor, _Arg> :
> _RangeAdaptorClosure<_Partial<_Adaptor, _Arg>>
> > - {
> > - _Arg _M_arg;
> > -
> > - template<typename _Tp>
> > - constexpr
> > - _Partial(int, _Tp&& __arg)
> > - : _M_arg(std::forward<_Tp>(__arg))
> > - { }
> > -
> > - template<typename _Range>
> > - requires __adaptor_invocable<_Adaptor, _Range, const _Arg&>
> > - constexpr auto
> > - operator()(_Range&& __r) const
> > - { return _Adaptor{}(std::forward<_Range>(__r), _M_arg); }
> > + { return _Binder::_S_call(_M_binder, std::forward<_Range>(__r)); }
> >
> > static constexpr bool _S_has_simple_call_op = true;
> > };
> > --
> > 2.51.0
> >
> >
