[libcxx-commits] [PATCH] D102037: [libcxx][views] Add drop_view.

Thu May 6 19:12:25 PDT 2021

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

Overall, this is great! Sadly, I can't LGTM till caching is implemented, but I think we'll be good to land this sometime next week :-)

================
Comment at: libcxx/include/__iterator/primitives.h:98
   template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
-  [[nodiscard]] constexpr iter_difference_t<_Ip> operator()(_Ip& __i, iter_difference_t<_Ip> __n, _Sp __bound) const {
     _LIBCPP_ASSERT(__n >= 0 || (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>),
----------------
If this is temporary, carry on. If there's a genuine concern, please highlight it in D101922.

================
Comment at: libcxx/include/__ranges/drop_view.h:33-37
+  template<class _Range>
+  concept __simple_view =
+    view<_Range> && range<const _Range> &&
+    same_as<sentinel_t<_Range>, iterator_t<const _Range>> &&
+    same_as<iterator_t<_Range>, sentinel_t<const _Range>>;
----------------
I'd very much appreciate a few simple libcxx tests for this concept. It's so important that I want so much as a light breeze nearby to cause a build failure.

================
Comment at: libcxx/include/__ranges/drop_view.h:41
+  class drop_view : public view_interface<drop_view<_View>> {
+    _View __base = _View();
+    range_difference_t<_View> __count = 0;
----------------
Nit 1: please suffix members with an underscore.
Nit 2: please put private stuff at the bottom unless it needs to be seen.

================
Comment at: libcxx/include/__ranges/drop_view.h:58-60
+      auto __tmp = ranges::begin(__base);
+      ranges::advance(__tmp, __count, ranges::end(__base));
+      return __tmp;
----------------
Let's hold off on merging this with main till `ranges::next` is available (I'll throw up a patch as soon as D101922 is approved).

You also need to cache the result for forward ranges (see [range.semi.wrap]). This is observable behaviour, so please add a test to ensure the caching happens.

================
Comment at: libcxx/include/__ranges/drop_view.h:65-67
+      auto __tmp = ranges::begin(__base);
+      ranges::advance(__tmp, __count, ranges::end(__base));
+      return __tmp;
----------------
I'd suggest wrapping this into a static `__begin_impl` so there's a shared impl. Same with `size`, but not `end`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:23
+
+int globalBuff[8];
+
----------------
Nit: I'd prefer if this were `std::array<int, 8>`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:27-66
+
+struct View : std::ranges::view_base {
+  int start;
+  constexpr View(int start = 0) : start(start) {}
+  constexpr View(View&&) = default;
+  constexpr View& operator=(View&&) = default;
+  constexpr friend int* begin(View& view) { return globalBuff + view.start; }
----------------
You have four views that satisfy `range<T const>`, if I'm not mistaken. Please ensure there's at least one non-const input view, and one non-const forward view too. Also, please check out `test/support/test_range.h` to see if some of these are already available (I don't think they are), and add some utilities there, as they'll be necessary in other range tests too.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:61-63
+struct InputView : std::ranges::view_base {
+  constexpr input_iterator<int*> begin() const { return input_iterator<int*>(globalBuff); }
+  constexpr int* end() const { return globalBuff + 8; }
----------------
`const`-qualified access member functions should probably return const-iterators. Similarly elsewhere.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:68
+
+constexpr bool operator==(input_iterator<int*> lhs, int* rhs) { return lhs.base() == rhs; }
+constexpr bool operator==(int* lhs, input_iterator<int*> rhs) { return rhs.base() == lhs; }
----------------
`cpp17_` or `cpp20_input_iterator`

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:77-79
+
+static_assert(ValidDropView<View>);
+static_assert(!ValidDropView<Range>);
----------------
Please also expand to check `const` qualification. This is //very// important!

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:80-84
+
+static_assert(std::same_as<decltype(ranges::drop_view(View(), 0)), ranges::drop_view<View>>);
+static_assert(std::same_as<decltype(ranges::drop_view(CopyableView(), 0)), ranges::drop_view<CopyableView>>);
+static_assert(std::same_as<decltype(ranges::drop_view(ForwardView(), 0)), ranges::drop_view<ForwardView>>);
+static_assert(std::same_as<decltype(ranges::drop_view(InputView(), 0)), ranges::drop_view<InputView>>);
----------------
These should possibly go into their own `ctad.compile.pass.cpp`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:110-135
+
+  {
+    // drop_view::begin
+    ranges::drop_view dropView1(View(), 4);
+    assert(dropView1.begin() == globalBuff + 4);
+
+    ranges::drop_view dropView2(ForwardView(), 4);
----------------
I'd prefer it if these were in functions such as `check_begin()`. The more intrinsic documentation we have, the less likely comments can go out of date.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:116-117
+
+    ranges::drop_view dropView2(ForwardView(), 4);
+    assert(dropView2.begin().base() == globalBuff + 4);
+
----------------
Can we test with some odd numbers and even non-powers of 2 too please?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:154-171
+
+  {
+    // drop_view::size
+    ranges::drop_view dropView1(View(), 4);
+    assert(dropView1.size() == 4);
+
+    ranges::drop_view dropView2(View(), 0);
----------------
We should have `const`-qualified tests for all these.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037