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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I think we're getting close. There still seem to be some open review comments and some comments marked as done, which aren't done. I added comment to them, be please verify whether I didn't overlook any. I still wonder about the visibility macros, but let's discuss that on Discord.



================
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&>
----------------
zoecarver wrote:
> tcanens wrote:
> > cjdb wrote:
> > > Should be `__not_same_as`.
> > Who came up with the idea to have a exposition-only concept named //`not-same-as`// that's not `not same_as`?
> This is the most annoying thing...
I agree, I also dislike this idea. Can you use the `__not_as_as`?


================
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> {
----------------
cjdb wrote:
> Regardless of how the `_LIBCPP_NODISCARD_EXT` discussion plays out, this one should be `[[nodiscard]]`.
Agreed, these 3 require `[[nodiscard]]` according to the Standard. (No idea why this `empty()` doesn't need it.)


================
Comment at: libcxx/include/__ranges/subrange.h:40
+  template<class _Range>
+  concept sized_range = range<_Range> && requires(_Range& t) {
+    ranges::size(t);
----------------
zoecarver wrote:
> Mordante wrote:
> > Please __uglify t.
> > Minor nit, since you use `_Range` instead of `_Tp`, how do you feel about using `__r` instead of `__t`?
> Good idea about using `__r`. I should have documented this more clearly, but I'm going to "properly" add these concepts in a future patch. I just didn't realize they were a prerequisite until too late here, so I just copied them in. 
Can you address this issue?


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp:12
+// UNSUPPORTED: gcc-10
+// XFAIL: msvc && clang
+
----------------
Is this msvc work-around still required. IIRC it was due to some __ugly_macro_conflicts.
Likewise for the other newly added unit tests.


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp:26
+static_assert(std::same_as<decltype(subrange(static_cast<int*>(nullptr), static_cast<int*>(nullptr), 0)), subrange<int*, int*, std::ranges::subrange_kind::sized>>);
+static_assert(std::same_as<decltype(subrange(static_cast<int*>(nullptr), nullptr, 0)), subrange<int*, nullptr_t, std::ranges::subrange_kind::sized>>);
+
----------------
Please add some linebreaks to keep this readable. Same for the other places with these overly long lines. IIRC our current maximum column width is 120.


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp:30
+//   4. Pointer elements are different types (fail)
+//   5. Pointer elements are same type (succeed)
+
----------------
Nice comment!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006