[libcxx-commits] [PATCH] D102006: [libcxx][ranges] Add range.subrange.

[Louis Dionne via Phabricator via libcxx-commits]

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

General note for the tests - can we split tests so that we test one overload (as documented in the Standard) per file? This is what we do elsewhere and it makes it easier to make sure we're testing everything correctly. If a function is only a very tiny derivative of another one, then it's OK to test both in one shot, of course.



================
Comment at: libcxx/include/__ranges/subrange.h:46
+      typename tuple_size<_Tp>::type; // Ensures `tuple_size<T>` is complete.
+      requires derived_from<tuple_size<_Tp>, integral_constant<size_t, 2>>;
+      typename tuple_element_t<0, remove_const_t<_Tp>>;
----------------
This is *so* weird - I would have expected something like `tuple_size<_Tp>::value == 2` instead. But leave as-is, that's how the Standard does it.


================
Comment at: libcxx/include/__ranges/subrange.h:64
+  struct __subrange_base {
+    static constexpr bool __store_size = false;
+    _Iter __begin = _Iter();
----------------
For a static like that, I'd use `_StoreSize` instead.

Re: using `_Iter` instead of `_Tp`, I strongly agree -- using `_Iter` is a lot clearer than what the Standard uses.


================
Comment at: libcxx/include/__ranges/subrange.h:65
+    static constexpr bool __store_size = false;
+    _Iter __begin = _Iter();
+    _Sent __end = _Sent();
----------------
`__begin_`? (same for other member variables)


================
Comment at: libcxx/include/__ranges/subrange.h:74
+  template<class _Iter, class _Sent>
+  struct __subrange_base<_Iter, _Sent, subrange_kind::sized> {
+    static constexpr bool __store_size = true;
----------------
Here, we're going to store the size if `_Kind == subrange_kind::sized` even if `_Sent` is a sized sentinel for `_Iter`. We should not be storing it when `_Sent` is a sized sentinel.


================
Comment at: libcxx/include/__ranges/subrange.h:97-99
+    using _Base::__store_size;
+    using _Base::__begin;
+    using _Base::__end;
----------------
Instead, you could use `this->member` when referring to a member in the dependent base, and `_Base::__store_size` when referring to the static. IMO that's easier to follow and more idiomatic, WDYT?


================
Comment at: libcxx/include/__ranges/subrange.h:142
+
+    [[nodiscard]] constexpr _Iter begin() requires (!copyable<_Iter>) {
+      return _VSTD::move(__begin);
----------------
I'm surprised they didn't make this `_Iter begin() &&` instead to make sure we can only call this on a xvalue.


================
Comment at: libcxx/include/__ranges/subrange.h:148
+
+    [[nodiscard]] constexpr bool empty() const { return __begin == __end; }
+
----------------
See, I'm very curious to understand why the Standard didn't mark this one as `[[nodiscard]]`. It can't be an oversight, since they marked a bunch of methods around it `[[nodiscard]]`. Just curious to understand what the motivation was.


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp:88
+
+constexpr bool testPrimatives() {
+  ranges::subrange<MoveOnlyForwardIter, int*> a(MoveOnlyForwardIter(globalBuff), globalBuff + 8, 8);
----------------
`testPrimitives`?


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp:21
+
+using std::ranges::subrange;
+
----------------
`namespace ranges = std::ranges` or something like that, but we frown upon importing names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006