[libcxx-commits] [PATCH] D105205: [libcxx][ranges] implements dangling, borrowed_iterator_t, borrowed_subrange_t

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

LGTM mod comments!



================
Comment at: libcxx/include/__ranges/borrowed_subrange_t.h:37-38
+
+  template<range _Rp>
+  using borrowed_subrange_t = typename __borrowed_subrange<_Rp>::type;
+} // namespace ranges
----------------
tcanens wrote:
> I'm under the impression that libc++ has a "better `conditional_t`" somewhere that doesn't require one class template instantiation per type?
Yes, and I think it's safe to use here because `_Rp` is already constrained to be a `range`, so `subrange<iterator_t<_Rp>>` is guaranteed well-formed. (If `_Rp` were allowed to be a non-range, then we couldn't use this trick: [godbolt](https://godbolt.org/z/do54PfYoz))
```
template<range _Rp>
using borrowed_subrange_t = _If<borrowed_range<_Rp>, subrange<iterator_t<_Rp>>, dangling>;
```


================
Comment at: libcxx/include/__ranges/dangling.h:34-41
+  template<class>
+  struct __borrowed_iterator { using type = dangling; };
+
+  template<borrowed_range _Rp>
+  struct __borrowed_iterator<_Rp> { using type = iterator_t<_Rp>; };
+
+  template<range _Rp>
----------------
```
template<range _Rp>
using borrowed_subrange_t = _If<borrowed_range<_Rp>, iterator_t<_Rp>, dangling>;
```



================
Comment at: libcxx/include/__ranges/subrange.h:21
 #include <__ranges/size.h>
-#include <__ranges/subrange.h>
 #include <__ranges/view_interface.h>
----------------
Yikes. I guess I need to audit `graph_header_deps.py`; it should have caught even this trivially circular inclusion.


================
Comment at: libcxx/include/ranges:133
 #include <__ranges/data.h>
+#include <__ranges/dangling.h>
 #include <__ranges/drop_view.h>
----------------
a-z plz (and also in the modulemap and CMakeLists)


================
Comment at: libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp:30
+template<class T>
+constexpr bool has_type = requires { typename std::ranges::borrowed_iterator_t<T>; };
+
----------------
As long as you're using `same_as` instead of `is_same_v`, you might as well also use `concept` instead of `constexpr bool` — it's shorter (and this line coincidentally is 2 characters too long :)).


================
Comment at: libcxx/test/std/ranges/range.utility/range.dangling/borrowed_subrange.compile.pass.cpp:23
+
+static_assert(std::same_as<std::ranges::borrowed_subrange_t<std::string>, std::ranges::dangling>);
+static_assert(std::same_as<std::ranges::borrowed_subrange_t<std::vector<int>>, std::ranges::dangling>);
----------------
IMO it's worth testing `std::ranges::borrowed_subrange_t<std::string&>` and `std::ranges::borrowed_subrange_t<std::string&&>`.

Using `string` and `string_view` as your archetypes for "borrowed" and "non-borrowed" seems like a good idea to me. Vice versa, assuming that we've already got a good test verifying that `std::ranges::borrowed_range<std::span<T>>` in the test suite for `span`, I think it'd be fine to eliminate all the lines here involving `<vector>` and `<span>`: they're too redundantly similar to the string/string_view lines.


================
Comment at: libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp:26-27
+
+struct S {};
+static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S>);
+
----------------
I'd lose lines 25-27 (as redundant) but add
```
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, int, int, int>);
static_assert(std::is_trivial_v<std::ranges::dangling>);
```
assuming that those properties are in fact intentional and/or mandated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105205