[libcxx-commits] [PATCH] D90999: [libc++] Implements ranges::enable_borrowed_range

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

In D90999#2381043 <https://reviews.llvm.org/D90999#2381043>, @miscco wrote:

> I am wondering whether we should split the ranges header up into a smaller part that we use to include in other headers adn the rest.
>
> Including all of ranges just for one trait seems a bit of an overkill.

I agree including the full ranges header in string_view and span feels a bit overkill. But I'll leave the decisions whether or not to split the header with the libc++ maintainers.



================
Comment at: libcxx/include/ranges:22
+    // [range.access], range access
+    inline constexpr unspecified begin = unspecified;
+    inline constexpr unspecified end = unspecified;
----------------
miscco wrote:
> I am wondering whether it is wise to copy the synopsis as it should be rather than how it is. This might trick the user to believe all of it is supported. What are the conventions here?
Looking at other headers it does they do the same for example concepts also uses 'unspecified' and 'see below'.


================
Comment at: libcxx/include/ranges:15
+
+#include <compare>              // see [compare.syn]
+#include <initializer_list>     // see [initializer.list.syn]
----------------
miscco wrote:
> I am slightly surprised that concepts is not required
Agreed. For this patch I don't need concepts, but `std::ranges::begin()` will require them.


================
Comment at: libcxx/include/ranges:325
+namespace ranges
+{
+
----------------
tschuett wrote:
> Why is the curly bracket not at the end of the line above?
Because that's what 'chrono' uses. However I have no objection against putting it on one line. Not sure what the libc++ coding style has to say about this.


================
Comment at: libcxx/test/std/ranges/range.range/enable_borrowed_range.pass.cpp:24
+{
+	static_assert(!std::ranges::enable_borrowed_range<char>);
+	static_assert(!std::ranges::enable_borrowed_range<int>);
----------------
miscco wrote:
> I believe you are missing coverage for all the ranges that are not borrowed, e.g. vector, list, ...
I'm not convinced they really need to be tested, but adding them is not much effort. So I'll add them.


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

https://reviews.llvm.org/D90999