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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM mod comments and passing CI!



================
Comment at: libcxx/include/__ranges/take_view.h:76
+          auto __size = size();
+          return counted_iterator{ranges::begin(__base_), static_cast<_DifferenceT>(__size)};
+        }
----------------
In all those `counted_iterator` constructions, the spec uses direct-initialization instead of list-initialization. I don't think this ever matters because there can't be a narrowing conversion from `_DifferenceT` to the argument of `counted_iterator::counted_iterator` (both types should always be exactly `iter_difference_t<iterator_t<_View>>`), but we might as well follow the spec.


================
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:
> cjdb wrote:
> > 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?
> Well, because we really want a static function that we pass `__base_` to, otherwise that kind of defeats the purpose of deducing `_View` vs `const _View`. 
> 
> If we're passing `__base_` we both lose `size` and have to move it into the function and then out of the function when we put it back into the member. That seems like an unreasonable cost. 
I think this would work:

```
template <class _Self>
_LIBCPP_HIDE_FROM_ABI
static constexpr auto __begin(_Self& __self) {
  using _MaybeConstView = _If<is_const_v<_Self>, _View const, _View>;
  if constexpr (sized_range< _MaybeConstView >) {
        if constexpr (random_access_range<_MaybeConstView>) {
          return ranges::begin(__self.__base_);
        } else {
          using _DifferenceT = range_difference_t<_MaybeConstView>;
          auto __size = __self.size();
          return counted_iterator{ranges::begin(__self.__base_), static_cast<_DifferenceT>(__size)};
        }
      } else {
        return counted_iterator{ranges::begin(__self.__base_), __self.__count_};
      }
}

_LIBCPP_HIDE_FROM_ABI
constexpr auto begin() requires (!__simple_view<_View>) {
  return take_view::__begin(*this);
}

// etc...
```

I'm still not sure whether I like that better than matching the spec 1:1. I'll leave it to you to decide @zoecarver .


================
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();
+    }
----------------
zoecarver wrote:
> ldionne wrote:
> > You could use `__rhs.__end_` here, which would avoid making a copy of the sentinel (`base()` makes a copy). Same applies above.
> That really //should// be inlined. But fair enough. I'm going to make it public so I can do that for lhs too. 
Depending on what the `_View`'s copy constructor does, it might not be possible to inline it, I think. I would definitely not rely on that, at least.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/base.pass.cpp:23
+
+struct ContiguousView : std::ranges::view_base {
+  int *ptr_;
----------------
Can you make this more minimal? For example, I think you could provide only `begin() const` as a member function.

Also, since you're using it in a bunch of places, perhaps you could put it in a `types.h` helper header in this directory? That would be a simple way to remove some duplication.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:96
+
+  {
+    std::ranges::take_view<SizedRandomAccessView> tv(SizedRandomAccessView{buffer}, 4);
----------------
Could you add a one-liner to each test to make it very obvious which branch of the `if constexpr` you're testing? Something as simple as:

```
// sized_range && random_access
```


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:132-142
+  {
+    std::ranges::take_view<CopyableView> tv(CopyableView{buffer}, 4);
+    assert(tv.begin() == std::counted_iterator<int*>(buffer, 4));
+    ASSERT_SAME_TYPE(decltype(tv.begin()), std::counted_iterator<int*>);
+  }
+
+  {
----------------
I'm not sure what coverage this is adding. Consider removing if it doesn't add anything.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctad.compile.pass.cpp:17
+#include <ranges>
+#include <cassert>
+
----------------
`#include <concepts>`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctad.compile.pass.cpp:45
+
+void testCTAD() {
+    View v;
----------------
Alternatively, you could leave at file scope. Whatever you prefer.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/ctor.pass.cpp:63
+  {
+    static_assert( std::default_initializable<DefaultConstructible<true>>);
+    static_assert(!std::default_initializable<DefaultConstructible<false>>);
----------------
You never actually default-construct an object here. Could you do that? Ideally, you'd then test that `base()` has been default-initialized, and that the `size()` is `0` (which you could do best by creating a view that has non-zero size when default-constructed).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/end.pass.cpp:96
+
+  {
+    std::ranges::take_view<SizedRandomAccessView> tv(SizedRandomAccessView{buffer}, 0);
----------------
Same comment as for `begin()` w.r.t. which `if` branch we're testing.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/size.pass.cpp:112
+  {
+    std::ranges::take_view<SizedForwardView> tv(SizedForwardView{buffer}, 8);
+    assert(tv.size() == 8);
----------------
You want to test this version (N == size-of-the-range) for const too.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/size.pass.cpp:117
+  {
+    const std::ranges::take_view<SizedForwardView> tv(SizedForwardView{buffer}, 10);
+    assert(tv.size() == 8);
----------------
You want to test this version (N > size-of-the-range) for non-const too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106507