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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 11 inline comments as done.
zoecarver added inline comments.


================
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) { }
----------------
cjdb wrote:
> I believe you also need an overload for when `__store_size == false`.
Other than the one on L112?


================
Comment at: libcxx/include/__ranges/subrange.h:134
+    constexpr subrange(_Range&& __range)
+      requires sized_range<_Range>
+      : subrange(__range, ranges::size(__range)) { }
----------------
cjdb wrote:
> 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.
Hmm, I think you're right. So that would be the case where `!sized_sentinel_for` but it had a `size` member or something?


================
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)
----------------
cjdb wrote:
> If we already have a `make_unsigned_like_t`, please use that. Otherwise, carry on.
I don't see one. 


================
Comment at: libcxx/include/__ranges/subrange.h:143
+      requires (_Kind == subrange_kind::sized)
+      : subrange(ranges::begin(__range), ranges::end(__range), __n) { }
+
----------------
tcanens wrote:
> cjdb wrote:
> > Standard uses braces for this one.
> > Standard uses braces for this one.
> 
> For ranges wording, wherever braces have behavior different from parens, the use of braces is almost certainly a defect. P2367R0 is expected to fix all such occurrences. https://github.com/cplusplus/draft/issues/4593 deals with the rest.
Is there an observable difference/do we care? If there's no difference I try to use parens, because that shows there's no difference (if I use braces, a reader might ask "is there a reason we have to use braces here?"). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006