[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors

Fri Jul 30 15:52:02 PDT 2021

ldionne added a comment.

In D107098#2917052 <https://reviews.llvm.org/D107098#2917052>, @tcanens wrote:

> The `decltype` part is important - when `_Op::__call(std::get<_Idxs>(std::move(__bound_))..., std::forward<_Args>(__args)...)` is ill-formed, this overload SFINAEs away, and if the `const&` overload is viable, that can be used, which ends up delivering lvalues.

Ooooookay, thanks to your help, I finally understood what was wrong and what the expected behavior should be. I think D107199 <https://reviews.llvm.org/D107199> should resolve these issues.

@Quuxplusone

> Based on my limited understanding of where p2387 actually is in the cycle, IMHO it would be very good to pull __bind_back out into its own PR and get it tested and pushed, essentially orthogonal to whatever else happens with this patch. bind_back's behavior seems pretty obvious, and thus very unlikely to change, and at least somewhat likely to get standardized into C++2b sooner or later.

I will pull it into its own PR and give it the tests it deserves. I just wanted to put the adaptor mechanism up for discussion so we can agree on how to do it, and then I'll split all the bits and pieces into separate PRs as makes sense.

================
Comment at: libcxx/include/__functional/bind_back.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > zoecarver wrote:
> > > > Quuxplusone wrote:
> > > > > High-level question: Why is this so complicated? All it needs to do AFAICT is just take the N arguments passed to it (for N=1 in C++20, but arguably we might see N>1 at some point), keep them in a tuple of `args...`, and then later take a `range auto&& lhs` and call the bound function `__f(lhs, args...)`. This doesn't feel like it ought to involve such a generic name as `__invoke_shuffle`.  Is this overengineered, or what am I missing? 
> > > > As I mentioned in discord, I have a much simpler implementation (that still allows us to have N arguments):
> > > > ```
> > > > template<class _Tuple, class _Idxs = typename __make_tuple_indices<tuple_size<_Tuple>::value>::type>
> > > > struct __bind_back_op;
> > > > 
> > > > template<class _Tuple, size_t... _Idxs>
> > > > struct __bind_back_op<_Tuple, __tuple_indices<_Idxs...>> {
> > > >     template <class _Fn, class _T2, class ..._Args>
> > > >     _LIBCPP_HIDE_FROM_ABI
> > > >     static constexpr auto __call(_Fn&& __f, _T2&& __t, _Args&& ...__args)
> > > >         noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)..., get<_Idxs>(__t)...)))
> > > >         -> decltype(      _VSTD::invoke(_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)..., get<_Idxs>(__t)...))
> > > >     { return              _VSTD::invoke(_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)..., get<_Idxs>(__t)...); }
> > > > };
> > > > 
> > > > template <class _Fn, class ..._Args, class = _EnableIf<
> > > >     conjunction_v<
> > > >         is_constructible<decay_t<_Fn>, _Fn>,
> > > >         is_move_constructible<decay_t<_Fn>>,
> > > >         is_constructible<decay_t<_Args>, _Args>...,
> > > >         is_move_constructible<decay_t<_Args>>...
> > > >     >
> > > > >>
> > > > _LIBCPP_HIDE_FROM_ABI
> > > > constexpr auto __bind_back(_Fn&& __f, _Args&&... __args)
> > > > {
> > > >     auto __t = tuple<decay_t<_Args>...>(__args...);
> > > >     return __perfect_forward<__bind_back_op<decltype(__t)>, _Fn, tuple<decay_t<_Args>...>>(_VSTD::forward<_Fn>(__f), __t);
> > > > }
> > > > ```
> > > Definitely seems simpler! Nits on your last three lines (IIUC):
> > > `auto __t = tuple<decay_t<_Args>...>(__args...);` should be `auto __t = tuple<decay_t<_Args>...>(_VSTD::forward<_Args>(__args)...);`
> > > `(_VSTD::forward<_Fn>(__f), __t)` should be `(_VSTD::forward<_Fn>(__f), _VSTD::move(__t))`
> > > I'm not sure if `get<_Idxs>(__t)...` needs to be `get<_Idxs>(_VSTD::forward<_T2>(__t))...` in each case or not.
> > @Quuxplusone 
> > > High-level question: Why is this so complicated? All it needs to do AFAICT is just take the N arguments passed to it (for N=1 in C++20, but arguably we might see N>1 at some point)
> > 
> > Well, I'm implementing this so that when `bind_back` is added, we already have it. And except for the "creating an additional tuple" trick suggested by Zoe (see tradeoffs below), I can't think of a simpler way to implement this.
> > 
> > @zoecarver The interesting idea here is to store the bound arguments in a tuple directly, and that indeed simplifies quite a few things. The downside is that, even after doing all the `std::forward`ing properly, we still end up creating an extra tuple with the bound arguments and moving it around. In other words, my implementation creates a single `tuple<Function, BoundArgs...>` (inside `__perfect_forward` and then invokes `Function` by shuffling the order in which `BoundArgs` is accessed.
> > 
> > Your implementation creates this: `tuple<Function, tuple<BoundArgs>>`. In doing so, we have an additional move-construction of a tuple, when you do this here: 
> > 
> > ```
> > return __perfect_forward<__bind_back_op<decltype(__t)>, _Fn, tuple<decay_t<_Args>...>>(_VSTD::forward<_Fn>(__f), tuple<decay_t<_Args>...>(std::forward<_Args>(__args)...));
> > ```
> > 
> > I'm not entirely sure which one is best - more complexity or an additional move construction?
> > 
> > Also, side comment: I won't get rid of the `__bind_back_t` type, even though doing so would yield fewer lines of code. I really want to hide `__perfect_forward` behind an opaque type so that error messages are a bit more understandable. You'd *much* rather see an error message involving `__bind_back_t<Args...>` than `__perfect_forward_impl<__bind_back_op<....>, Args...>`.
> > 
> > Since I'm not sure which implementation to go with yet, I'm putting the diff between our two implementations here: https://gist.github.com/ldionne/a8ca54b04a158748ddb8a7696d701ef1. I can apply it or discard it once we've agreed on something here.
> > 
> > So, I think the question is: Is it acceptable to trigger an additional move-construction of the tuple that contains bound arguments? FWIW, I have extensive experience with these sorts of things from when I was working on Boost.Hana (where basically everything is tuples moving around), and I remember that at the time, we could sometimes get pretty terrible codegen when we were moving around tuples of trivial types (so imagine non-trivial types). I don't know whether that's still relevant though, it was a couple years ago.
> > 
> > Also, in the expression `__perfect_forward<__bind_back_op<decltype(__t)>, _Fn, tuple<decay_t<_Args>...>>(_VSTD::forward<_Fn>(__f), tuple<decay_t<_Args>...>(std::forward<_Args>(__args)...))`, there's nothing that mandates in-place construction of the `tuple` inside the `__perfect_forward` object, right? The C++20 rules for copy elision wouldn't have an influence on that, would it (if so, then my concern about an additional move is moot)? @Quuxplusone
> > Why is this so complicated?
> I didn't know this was basically implementing [p2387r0 "Pipe support for user-defined range adaptors
> "](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2387r0.html). It would be helpful to add that paper number to the title/summary, and also elaborate in the summary on any bits that //aren't// quite conforming to p2387 (except of course for some double-underscores here and there because it's not standard yet).
> IIUC, an argument against @zoecarver's "simpler" approach is simply that it doesn't precisely implement p2387, and we want to implement p2387? Is that an accurate assessment of the situation?
> 
> Based on my limited understanding of where p2387 actually is in the cycle, IMHO it would be very good to pull `__bind_back` out into its own PR and get it tested and pushed, essentially orthogonal to whatever else happens with this patch. `bind_back`'s behavior seems pretty obvious, and thus //very// unlikely to change, and at least //somewhat// likely to get standardized into C++2b sooner or later.
> 
> @ldionne:
> > there's nothing that mandates in-place construction...?
> 
> Correct. If `__perfect_forward<F, Args...>` were an aggregate, then that syntax //would// mandate in-place construction (even with the parens, as of C++20); but it's not an aggregate (I checked just now against `is_aggregate_v`). Also, FYI, it's not trivially copyable even when `Args...` are trivially copyable; that kind of sucks. It'd be an ABI break to change now?
>  It would be helpful to add that paper number to the title/summary, and also elaborate in the summary on any bits

Will do.

> IIUC, an argument against @zoecarver's "simpler" approach is simply that it doesn't precisely implement p2387, and we want to implement p2387? Is that an accurate assessment of the situation?

No. The Standard doesn't mention whether we're allowed to create a temporary tuple or now. I'm confident we are. It's just about QOI. However, I think we found a solution that was both efficient and simple, see the next update.

> Also, FYI, it's not trivially copyable even when Args... are trivially copyable; that kind of sucks. It'd be an ABI break to change now?

Yes, it would technically be an ABI break because we use this in `not_fn`. But honestly, it's probably a very very benign ABI break since most people are probably not passing that unspecified type across an ABI boundary. And if they are, they're kind of crazy. If we had a compelling way to make it trivial, I'd be open to that discussion. Unfortunately, I kind of like using `std::tuple` here in the implementation, and that alone prevents it from being trivially copyable.

================
Comment at: libcxx/include/__utility/integer_sequence.h:97
+template<class ..._Sequences>
+using __join_integer_sequences = typename __join_integer_sequences_impl<_Sequences...>::type;
+
----------------
Quuxplusone wrote:
> Naming bikeshed: `__integer_sequence_cat` for consistency with `tuple_cat`?
This is going away!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107098/new/

https://reviews.llvm.org/D107098