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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I have not looked at the tests yet.



================
Comment at: libcxx/include/__ranges/take_view.h:34
+  class take_view : public view_interface<take_view<_View>> {
+    _View __base_ = _View();
+    range_difference_t<_View> __count_ = 0;
----------------
`[[no_unique_address]]`


================
Comment at: libcxx/include/__ranges/take_view.h:40
+  public:
+    take_view() = default;
+
----------------
`requires default_initializable<_View>`, and also `_LIBCPP_HIDE_FROM_ABI`.


================
Comment at: libcxx/include/__ranges/take_view.h:73
+          auto __size = size();
+          return counted_iterator{ranges::begin(__base_), static_cast<range_difference_t<const _View>>(__size)};
+        }
----------------
`using _Difference = range_difference_t<const _View>;`? (applies above)


================
Comment at: libcxx/include/__ranges/take_view.h:110
+      auto __n = ranges::size(__base_);
+      return _VSTD::min(__n, static_cast<decltype(__n)>(__count_));
+    }
----------------
cjdb wrote:
> 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.
It seems conforming to me to use `std::min` here, no?


================
Comment at: libcxx/include/__ranges/take_view.h:126
+    using _Iter = counted_iterator<iterator_t<__maybe_const<_OtherConst, _View>>>;
+    sentinel_t<_Base> __end_ = sentinel_t<_Base>();
+
----------------
`[[no_unique_address]]`


================
Comment at: libcxx/include/__ranges/take_view.h:132
+public:
+    __sentinel() = default;
+
----------------
`_LIBCPP_HIDE_FROM_ABI`


================
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?
+
----------------
cjdb wrote:
> 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.
I say we should move it. I don't see a reason either.


================
Comment at: libcxx/include/__ranges/take_view.h:154
+    friend constexpr bool operator==(const _Iter<_Const>& __lhs, const __sentinel& __rhs) {
+      return __lhs.count() == 0 || __lhs.base() == __rhs.base();
+    }
----------------
You could use `__rhs.__end_` here, which would avoid making a copy of the sentinel (`base()` makes a copy). Same applies above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106507