[libcxx-commits] [PATCH] D106507: [libcxx][ranges] Add ranges::take_view.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/take_view.h:18
+#include <__ranges/view_interface.h>
+#include <algorithm>
+
----------------
Please replace with the specific algorithms you use.


================
Comment at: libcxx/include/__ranges/take_view.h:43
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr take_view(_View __base, range_difference_t<_View> __count)
+      : __base_(_VSTD::move(__base)), __count_(__count) {}
----------------
Can be `const`.


================
Comment at: libcxx/include/__ranges/take_view.h:53
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr auto begin() requires (!__simple_view<_View>) {
+      if constexpr (sized_range<_View>) {
----------------
zoecarver wrote:
> Chris, before you ask, I did try to factor this out into a static member function, but ran into a couple of issues (first, I had to inline `size`, second, it didn't work well for move only views). 
> 
> 😉 
Now that you've brought it to my attention... ;)

* why did you need to inline `size`?
* why doesn't it work well for move-only views?


================
Comment at: libcxx/include/__ranges/take_view.h:110
+      auto __n = ranges::size(__base_);
+      return _VSTD::min(__n, static_cast<decltype(__n)>(__count_));
+    }
----------------
Hmm... I just realised that if we're going to be using algorithms here, we should definitely be using ranges overloads. You won't be stepping on my turf if you implement the min/max family.


================
Comment at: libcxx/include/__ranges/take_view.h:135
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit __sentinel(sentinel_t<_Base> __end) : __end_(__end) {} // TODO: why not move this one too?
+
----------------
zoecarver wrote:
> We move `__end` in the other constructor, why not here too? We don't use it later. 
If it's passed by value we should be allowed to do so.


================
Comment at: libcxx/include/__ranges/take_view.h:159
+  template<class _Range>
+  take_view(_Range&&, range_difference_t<_Range>) -> take_view<views::all_t<_Range>>;
+
----------------
Oooooh nice, `all_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106507