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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__functional/bind_back.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
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.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:30
+template <class _Tp>
+struct __range_adaptor_closure { };
+
----------------
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/ )


================
Comment at: libcxx/include/__ranges/transform_view.h:422
+  inline constexpr auto transform = __transform_adaptor{};
+} // end namespace views
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098