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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline 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
----------------
ldionne wrote:
> Quuxplusone wrote:
> > 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>;
> > ```
> You can either use `_If` or just plain `conditional_t`. `_If` creates fewer instantiations.
Fewer instantiations is always good.


================
Comment at: libcxx/include/ranges:101
+  template<class I, class S, subrange_kind K>
+    inline constexpr bool enable_borrowed_range<subrange<I, S, K>> = true;
+
----------------
zoecarver wrote:
> Is this implemented? It doesn't look like it, yet. 
This was the first thing implemented in `<ranges>` (see line 136).


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


================
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>; };
+
----------------
Quuxplusone wrote:
> 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 :)).
`has_X` is a trait, not a concept :)


================
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>);
----------------
Quuxplusone wrote:
> 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.
> IMO it's worth testing `std::ranges::borrowed_subrange_t<std::string&>` and `std::ranges::borrowed_subrange_t<std::string&&>`.

Yes, good idea (also applied to `borrowed_iterator`).

> 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.

I'd still prefer to keep the other types to make sure `borrowed_X` isn't hard-coded to a single type.


================
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>);
+
----------------
Quuxplusone wrote:
> 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.
> 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>);
> ```

Keeping lines 25-7 (making sure this works with user-defined types is important), and added both your lines.

> assuming that those properties are in fact intentional and/or mandated.

🤷 The constructor is variadic, so it's probably intentional and mandated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105205