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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

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?



================
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>>...
+    >
----------------
Isn't `conjunction` really expensive to use and non-short circuiting?


================
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
----------------
`__is_range_adaptor_closure` should either be renamed to remove `is` and replace `class`, or should be a `constexpr bool`.


================
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
----------------
Why is this an atomic copnstraint?


================
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>; };
-
----------------
This concept's addition and removal seems... arbitrary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098