[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jul 30 09:39:39 PDT 2021
ldionne marked 15 inline comments as done.
ldionne added a comment.
In D107098#2915589 <https://reviews.llvm.org/D107098#2915589>, @tcanens wrote:
> Just a quick note: when P2281R1 <https://wg21.link/P2281R1> talks about perfect forwarding call wrappers, it really means perfect forwarding call wrappers exactly as specified in the standard <http://eel.is/c++draft/func.require#def:perfect_forwarding_call_wrapper>, that is:
>
>> A //perfect forwarding call wrapper// is an argument forwarding call wrapper that forwards its state entities to the underlying call expression. This forwarding step delivers a state entity of type `T` as `cv T&` when the call is performed on an lvalue of the call wrapper type and as `cv T&&` otherwise, where `cv` represents the cv-qualifiers of the call wrapper and where `cv` shall be neither `volatile` nor `const volatile`.
>
> In particular, invoking an rvalue wrapper must deliver the state entity as an xvalue and cannot fall back to const lvalue the way `__perfect_forward` does (without deducing `this`, I believe that this requires additional deleted `operator()` overloads). This is particularly significant for `split` - see the examples in the paper.
Thanks a lot for the heads up. However, I'm not entirely sure I understand where the issue is, because what I'm seeing for `__perfect_forward` is this (leaving aside `noexcept` and `-> decltype`):
template<class... _Args>
constexpr auto operator()(_Args&&... __args) &&
{return _Op::__call(std::get<_Idxs>(std::move(__bound_))..., std::forward<_Args>(__args)...);}
Since we call `std::get` on `std::move(tuple)`, I think we end up passing rvalue references to `__call`, don't we? Or did you mean something else?
In D107098#2915532 <https://reviews.llvm.org/D107098#2915532>, @cjdb wrote:
> There are a //lot// of moving parts here and it's going to take me a while to digest everything that's happening (so expect multiple rounds of commentary over a few days). I don't recall my pipe design being as complex; perhaps we should set up a 1:1 to chat about the differences between our respective designs?
FWIW, if you take out the helpers, there really isn't that much going on. We basically have one CRTP base that enables piping (`__range_adaptor_closure`), and then one helper class that n-ary adaptors can use to create partially applied closures (`__partial_range_adaptor_closure`). `__composed_range_adaptor_closure` is "local", IOW it's only used for the composing `operator|`, but nobody outside of that file needs it.
That being said, I just found a way to eliminate both `__partial_range_adaptor_closure` and `__composed_range_adaptor_closure` by making something slightly more general.
================
Comment at: libcxx/include/__functional/bind_back.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
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
================
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>>...
+ >
----------------
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.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:29-33
+template <class _Tp>
+struct __range_adaptor_closure { };
+
+template <class _Tp>
+concept __is_range_adaptor_closure = derived_from<_Tp, __range_adaptor_closure<_Tp>>;
----------------
Quuxplusone wrote:
> I think it's good that you picked `all` and `transform` for this PR; that's one "nullary" adaptor and one "unary" adaptor. I'm still confused how you're managing to get away with just one `__range_adaptor_closure` template; it intuitively feels like you should just have one `__nullary_range_adaptor` for things invoked as `rng | x` and one `__unary_range_adaptor` for things invoked as `rng | x(a)`. (These are the only two types of adaptors AFAIK.)
> How do you avoid making `rng | views::all()` well-formed?
> ...
> Ah, I'm slowly seeing it. Your `__range_adaptor_closure` is something that can be piped into; so `views::all` IS-A `__range_adaptor_closure`, and `views::transform` has an `operator()` that //returns// a `__range_adaptor_closure`. But then why does `views::transform` itself inherit from `__range_adaptor_closure`? Does that make `rng | views::transform` (with no args) compile when it shouldn't? Why is that inheritance there?
> I think it's good that you picked all and transform for this PR; that's one "nullary" adaptor and one "unary" adaptor.
Yes, that was the goal.
> it intuitively feels like you should just have one `__nullary_range_adaptor` for things invoked as `rng | x` and one `__unary_range_adaptor` for things invoked as `rng | x(a)`. (These are the only two types of adaptors AFAIK.)
With http://wg21.link/p2387, it is conceivable that we'd have adaptors that take more than one argument, so that's why I support those more generically. You're right, supporting only nullary or unary adaptors would have been easier (basically it doesn't require implementing a general `bind_back` mechanism).
> How do you avoid making `rng | views::all()` well-formed?
`views::all()` is not well-formed itself. You *have* to provide at least one argument if you're going to call `views::all`.
> But then why does `views::transform` itself inherit from `__range_adaptor_closure`?
Hmm, you're right, it shouldn't. I fixed it and added a test.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:30
+template <class _Tp>
+struct __range_adaptor_closure { };
+
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Why does this need to be a template?
> Probably because EBO, specifically the "empty-base exclusion principle." You don't want to have multiple types A,B with the same empty base type, especially when A might have B as its first member.
> ( https://quuxplusone.github.io/blog/2021/05/07/std-iterator-as-a-base-class/ )
There's what Arthur says, and also I'm sticking closely to http://wg21.link/p2387 so that it's trivial to implement once/if it gets voted in.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:65
+template <ranges::viewable_range _View, class _Closure>
+ requires __is_range_adaptor_closure<decay_t<_Closure>>
+_LIBCPP_HIDE_FROM_ABI
----------------
cjdb wrote:
> `__is_range_adaptor_closure` should either be renamed to remove `is` and replace `class`, or should be a `constexpr bool`.
As I explain below, `__is_range_adaptor_closure` started as a `constexpr bool`, and then I changed it to a `concept` per Barry's paper. I can't just call it `__range_adaptor_closure` since that would conflict with the class of the same name, but I'll find another name.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:73-74
+template <class _Closure1, class _Closure2>
+ requires (__is_range_adaptor_closure<decay_t<_Closure1>> &&
+ __is_range_adaptor_closure<decay_t<_Closure2>>)
+_LIBCPP_HIDE_FROM_ABI
----------------
cjdb wrote:
> Why is this an atomic copnstraint?
You're right, it shouldn't. Actually, the code started with `__is_range_adaptor_closure` being a `constexpr bool`, so then I had to do this. When I changed it to being a `concept`, I didn't update that code. Fixed.
================
Comment at: libcxx/include/__ranges/transform_view.h:422
+ inline constexpr auto transform = __transform_adaptor{};
+} // end namespace views
+
----------------
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.
================
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" }
----------------
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.)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:10-11
-template<class T, class F>
-concept ValidDropView = requires { typename std::ranges::transform_view<T, F>; };
-
----------------
cjdb wrote:
> This concept's addition and removal seems... arbitrary.
It's a copy-paste error (@zoecarver). Those are the tests for `transform_view`, not `drop_view`, and it's not used anywhere. But actually, now that I revisit that, I think what we want instead is to rename it to `ValidTransformView` and test it somewhere. Done as a NFC (7b3ada712aff7254791b1c8cf905361fc478b70d).
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