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

[Christopher Di Bella via Phabricator via libcxx-commits]

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

I'll review the tests later today, but I anticipate some of my feedback might shape the existing tests.
I'd also appreciate if we can split this test set into multiple files, similar to how `test/std/containers/sequences/vector/` is broken down.



================
Comment at: libcxx/include/__ranges/concepts.h:72
+template<class _Tp, class _Up>
+concept __not_same_as = !same_as<remove_cvref_t<_Tp>, remove_cvref_t<_Up>>;
+
----------------
zoecarver wrote:
> Mordante wrote:
> > Why not in the namespace?
> I figured this is a more general utility. Not just for ranges. 
> 
> But this makes me think: it would probably be better to move this to `<concepts>`, actually. 
Yes, please move to `<concepts>`.


================
Comment at: libcxx/include/__ranges/subrange.h:48-51
+    // If they're both pointers, they must have the same element type.
+    !(is_pointer_v<decay_t<_From>> &&
+      is_pointer_v<decay_t<_To>> &&
+      __not_same_as<remove_pointer_t<decay_t<_From>>, remove_pointer_t<decay_t<_To>>>);
----------------
Checking that the comment isn't the opposite of what the code wants is a bit convoluted. I suggest making a named concept out of this bit to improve readability.


================
Comment at: libcxx/include/__ranges/subrange.h:56
+    !is_reference_v<_Tp> && requires(_Tp __t) {
+      typename tuple_size<_Tp>::type;
+      requires derived_from<tuple_size<_Tp>, integral_constant<size_t, 2>>;
----------------
I think the standard-provided comment is useful in improving readability.


================
Comment at: libcxx/include/__ranges/subrange.h:115-119
+
+    constexpr subrange(__convertible_to_non_slicing<_Iter> auto __iter, _Sent __sent,
+                       make_unsigned_t<iter_difference_t<_Iter>> __n)
+      requires (_Kind == subrange_kind::sized)
+      : _Base(_VSTD::move(__iter), __sent, __n) { }
----------------
I believe you also need an overload for when `__store_size == false`.


================
Comment at: libcxx/include/__ranges/subrange.h:134
+    constexpr subrange(_Range&& __range)
+      requires sized_range<_Range>
+      : subrange(__range, ranges::size(__range)) { }
----------------
You'll probably need to guard on `!__store_size && sized_range<R>`, since it's possible for both to be true, thus creating an ambiguity. Please add a test for this very case.


================
Comment at: libcxx/include/__ranges/subrange.h:141
+               convertible_to<sentinel_t<_Range>, _Sent>
+    constexpr subrange(_Range&& __range, make_unsigned_t<iter_difference_t<_Iter>> __n)
+      requires (_Kind == subrange_kind::sized)
----------------
If we already have a `make_unsigned_like_t`, please use that. Otherwise, carry on.


================
Comment at: libcxx/include/__ranges/subrange.h:143
+      requires (_Kind == subrange_kind::sized)
+      : subrange(ranges::begin(__range), ranges::end(__range), __n) { }
+
----------------
Standard uses braces for this one.


================
Comment at: libcxx/include/__ranges/subrange.h:146
+    template<class _Pair>
+      requires (!same_as<subrange, _Pair>) &&
+               __pair_like_convertible_from<_Pair, const _Iter&, const _Sent&>
----------------
Should be `__not_same_as`.


================
Comment at: libcxx/include/__ranges/subrange.h:154
+
+    _LIBCPP_NODISCARD_EXT constexpr _Iter begin() requires (!copyable<_Iter>) {
+      return _VSTD::move(__begin);
----------------
Regardless of how the `_LIBCPP_NODISCARD_EXT` discussion plays out, this one should be `[[nodiscard]]`.


================
Comment at: libcxx/include/__ranges/subrange.h:171
+
+    _LIBCPP_NODISCARD_EXT constexpr subrange next(iter_difference_t<_Iter> __n = 1) const&
+      requires forward_iterator<_Iter> {
----------------
Regardless of how the `_LIBCPP_NODISCARD_EXT` discussion plays out, this one should be `[[nodiscard]]`.


================
Comment at: libcxx/include/__ranges/subrange.h:178
+
+    _LIBCPP_NODISCARD_EXT constexpr subrange next(iter_difference_t<_Iter> __n = 1) && {
+      advance(__n);
----------------
Regardless of how the `_LIBCPP_NODISCARD_EXT` discussion plays out, this one should be `[[nodiscard]]`.


================
Comment at: libcxx/include/__ranges/subrange.h:183
+
+    _LIBCPP_NODISCARD_EXT constexpr subrange prev(iter_difference_t<_Iter> __n = 1) const
+      requires bidirectional_iterator<_Iter> {
----------------
Regardless of how the `_LIBCPP_NODISCARD_EXT` discussion plays out, this one should be `[[nodiscard]]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006