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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


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


================
Comment at: libcxx/include/__ranges/range_adaptor.h:30
+template <class _Tp>
+struct __range_adaptor_closure { };
+
----------------
Why does this need to be a template?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098