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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:30
+template <class _Tp>
+struct __range_adaptor_closure { };
+
----------------
ldionne wrote:
> 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.
A heads up: I expect P2387 to undergo a few revisions before it gets to LEWG, let alone LWG, so we shouldn't marry ourselves to P2387R0.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:36
+template <class _Tp>
+concept _RangeAdaptorClosure = derived_from<_Tp, __range_adaptor_closure<_Tp>>;
+
----------------
I think `__pipeable` is a better name in this case.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:45-46
+
+template <ranges::viewable_range _View, class _Closure>
+    requires _RangeAdaptorClosure<decay_t<_Closure>>
+_LIBCPP_HIDE_FROM_ABI
----------------



================
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)); }
----------------
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.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:53-55
+template <class _Closure1, class _Closure2>
+    requires _RangeAdaptorClosure<decay_t<_Closure1>> &&
+             _RangeAdaptorClosure<decay_t<_Closure2>>
----------------



================
Comment at: libcxx/include/__ranges/transform_view.h:404-409
 } // namespace ranges
 
+namespace views {
+namespace __transform {
+  struct __fn {
+    template<ranges::viewable_range _Range, class _Fn>
----------------
There's not really a point in us doing the checks twice, but if you're wanting to do them, we should probably do them properly.


================
Comment at: libcxx/include/__ranges/transform_view.h:428
+}
+} // end namespace views
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098