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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:45-60
+template <ranges::viewable_range _View, class _Closure>
+    requires _RangeAdaptorClosure<decay_t<_Closure>>
+_LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
+-> decltype(      _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)))
+{ return          _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)); }
----------------
ldionne wrote:
> cjdb wrote:
> > cjdb wrote:
> > > cjdb wrote:
> > > > 
> > > I'd //really// like for us to make these hidden friends. It's definitely doable for `range | closure`, and I //think// it's doable for `closure | closure` too.
> > What am I saying? If it's possible for `range | closure`, of course it's possible for `closure | closure` :facepalm:
> This is what we'd end up with:
> 
> ```
> template <class _Closure>
> struct __range_adaptor_closure {
>     template <ranges::viewable_range _View>
>     _LIBCPP_HIDE_FROM_ABI
>     friend constexpr auto operator|(_View&& __view, _Closure&& __closure)
>     noexcept(noexcept(_VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view))))
>     -> decltype(      _VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view)))
>     { return          _VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view)); }
> 
>     template <ranges::viewable_range _View>
>     _LIBCPP_HIDE_FROM_ABI
>     friend constexpr auto operator|(_View&& __view, _Closure const& __closure)
>     noexcept(noexcept(_VSTD::invoke(__closure, _VSTD::forward<_View>(__view))))
>     -> decltype(      _VSTD::invoke(__closure, _VSTD::forward<_View>(__view)))
>     { return          _VSTD::invoke(__closure, _VSTD::forward<_View>(__view)); }
> 
>     template <_RangeAdaptorClosure _OtherClosure>
>     _LIBCPP_HIDE_FROM_ABI
>     constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
>     noexcept(noexcept(__range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1)))))
>     -> decltype(      __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1))))
>     { return          __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1))); }
> 
>     template <_RangeAdaptorClosure _OtherClosure>
>     _LIBCPP_HIDE_FROM_ABI
>     constexpr auto operator|(_Closure const& __c1, _OtherClosure&& __c2)
>     noexcept(noexcept(__range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1))))
>     -> decltype(      __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1)))
>     { return          __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1)); }
> };
> ```
> 
> A few remarks:
> 
> - For `range | closure`, it's not clear to me that we gain much since we need two overloads. And perhaps we'd actually need more to handle `_Closure&` and `_Closure const&&`?
> - For `closure | closure`, that simply doesn't work because that will cause an ambiguity. If we have two closures `Closure1 c1` and `Closure2 c2`, `c1 | c2` could use either the `operator|(Closure1, auto)` injected by `__range_adaptor_closure<Closure1>`, or the `operator|(Closure2, auto)` injected by `__range_adaptor_closure<Closure2>`. I don't see an easy way to solve that problem.
> 
> For those reasons, I'd be tempted to leave both as-is.
[[ https://godbolt.org/z/az96scex5 | Here's a minimised version ]] of what I have in mind. I don't think there's a lot you'll need to do to integrate it: just decorations and seeing if your existing concepts have a mapping (I couldn't see it: from what I could tell, we need both yours and mine).


================
Comment at: libcxx/include/__ranges/range_adaptor.h:45-49
+template <ranges::viewable_range _View, _RangeAdaptorClosure _Closure>
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
+-> decltype(      _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)))
----------------
Unless I'm mistaken, the decltypes are only there for SFINAE friendliness?


================
Comment at: libcxx/include/__ranges/range_adaptor.h:56
+noexcept(noexcept(__range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_Closure2>(__c2), _VSTD::forward<_Closure1>(__c1)))))
+-> decltype(      __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_Closure2>(__c2), _VSTD::forward<_Closure1>(__c1))))
+{ return          __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_Closure2>(__c2), _VSTD::forward<_Closure1>(__c1))); }
----------------
Ditto, but you'll also need a `constructible_from` for this one.


================
Comment at: libcxx/include/__ranges/transform_view.h:414
+    noexcept(noexcept(transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f))))
+    -> decltype(      transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)))
+    { return          transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)); }
----------------
Same dealeo as above: this should be a requirement, not expression-SFINAE. Gonna leave it at three comments, I trust you to find all the rest :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098