[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jul 30 10:47:42 PDT 2021
Quuxplusone added inline comments.
================
Comment at: libcxx/include/__functional/bind_back.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
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?
================
Comment at: libcxx/include/__functional/bind_back.h:62-67
+ 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>>...
+ >
----------------
ldionne wrote:
> cjdb wrote:
> > Isn't `conjunction` really expensive to use and non-short circuiting?
> `conjunction` is short-circuiting. We implement it with `_And`, which is our internal helper and is as fast as we're able to make it.
Nit: FWIW, I'd prefer `_And` over `conjunction_v` just for readability. But that was the kind of style comment I wasn't going to make except now we've started talking about it. ;)
================
Comment at: libcxx/include/__ranges/transform_view.h:422
+ inline constexpr auto transform = __transform_adaptor{};
+} // end namespace views
+
----------------
ldionne wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > In `all.h`, we create a nested namespace `views::__all`, place `struct __fn` in there, and then define the niebloid itself in
> > > > ```
> > > > inline namespace __cpo {
> > > > inline constexpr auto all = __all::__fn{};
> > > > } // namespace __cpo
> > > > ```
> > > > (all this basically to avoid unintentional ADL when `views::all` is passed as a function argument). If there's no technical reason to diverge here, please stick to that pattern; it'll make it easier to tell what's significant in this PR.
> > > Not sure if this is UB, but we can actually see this trigger a ODR violation like so:
> > > ```
> > > namespace std { namespace views {
> > >
> > > struct dummy {
> > > friend void transform() { }
> > > };
> > >
> > > }}
> > > ```
> > > (If it's not UB we should definitely add those as tests.)
> > >
> > > ---
> > >
> > > Refs (why we put all niebloids into their own inline namespace): https://stackoverflow.com/questions/50010074/why-does-range-v3-put-its-function-objects-into-an-inline-namespace and https://github.com/ericniebler/range-v3/pull/423
> > I'm confident that the user-programmer is not allowed to add their own `struct dummy` into libc++'s `namespace std::views`. So this would be IFNDR if they tried it. (Technically the symptom wouldn't be ODR-violation but simply "`transform` has already been declared as a different kind of entity.")
> > But yeah, replace the name `transform` with `swap` and this is exactly the sort of thing the library itself needs to be able to do. Barry's top SO answer is spot-on. And I was totally wrong about this being ADL-related.
> There's a lot here.
>
> @Quuxplusone
> > In `all.h`, we create a nested namespace `views::__all`, place `struct __fn` in there, and then define the niebloid itself in [...]
>
> We don't have other types with a `friend auto all()` function in them, which would clash with `views::all` and mandate the use of the `inline namespace` trick. So I don't think there is an actual reason to define it like the other CPOs, and it would be simpler and cleaner not to. I think we might just have cargo-culted niebloids a little bit. I just proposed changing how we define `views::all` in D107169.
>
> If I'm misunderstanding something, please let me know in D107169. But otherwise, I think both `views::transform` and `views::all` (and all the other non-CPOs) should be defined as simply as possible, without an inline namespace.
>
> @zoecarver
> > Not sure if this is UB, but we can actually see this trigger a ODR violation like so: [...]
>
> As Arthur said, folks are not allowed to add such things in namespace `std`, so this can't happen.
(This has now been adjusted to match my preference and the thread is closed AFAIC; thanks.)
================
Comment at: libcxx/include/__utility/integer_sequence.h:97
+template<class ..._Sequences>
+using __join_integer_sequences = typename __join_integer_sequences_impl<_Sequences...>::type;
+
----------------
Naming bikeshed: `__integer_sequence_cat` for consistency with `tuple_cat`?
================
Comment at: libcxx/include/module.modulemap:422
module bind { private header "__functional/bind.h" }
+ module bind_back { private header "__functional/bind_back.h" }
module bind_front { private header "__functional/bind_front.h" }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Naming nit: It looks like our style for implementation-detail headers is `module __bind_back` and `__functional/__bind_back.h`. Compare to `__decay_copy.h` below; and I've proliferated the underscored style in my [cmp.alg] patch.
> > (However, `perfect_forward.h` seems to break the pattern already.)
> I thought we had actually agreed on the contrary, that is we don't name headers with a leading `__`? I'm pretty sure we had that discussion in a prior review. We certainly follow that convention (no leading `__` for several headers, like `__memory/compressed_pair.h`, etc.)
I suppose omit-the-underscores does make it simpler for things that are expected to become standard. When `__bind_back` gains a `bind_back` facade in C++2b, we don't have to update the filename, just add ~5 lines at the bottom of the existing file.
That's a great reason to omit the underscores here. I'm ambivalent as to whether it affects e.g. `__boolean_testable.h` (or even `__decay_copy.h` now that a core-language solution to the latter is being pursued in [p0849] so the library version will probably never be standardized). Anyway, let's take this tangent to D107036.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107098/new/
https://reviews.llvm.org/D107098
More information about the libcxx-commits
mailing list