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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/dangling.h:32
+  dangling() = default;
+  constexpr dangling(auto&&...) noexcept {}
+};
----------------
`_LIBCPP_HIDE_FROM_ABI`


================
Comment at: libcxx/include/__ranges/subrange.h:22
 #include <__ranges/size.h>
 #include <__ranges/view_interface.h>
 #include <concepts>
----------------
You need `<type_traits>` for `_If` (here and in `dangling.h`).


================
Comment at: libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp:32
+template <class T>
+constexpr bool has_type = requires {
+  typename std::ranges::borrowed_iterator_t<T>;
----------------
zoecarver wrote:
> While you're at it (changing to a concept) can you rename this to something more descriptive? Maybe something like "hasIterator" or "validForBorrowedIter" or something that indicates you're checking the requirements (that T is a range).
Agreed, I would just use 

```
template <class T>
concept has_borrowed_iterator = requires {
  typename std::ranges::borrowed_iterator_t<T>;
};
```

That's closest to what we do in other tests, no?


================
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>);
+
----------------
cjdb wrote:
> 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?
Instead, why not:

```
template<int> struct S { };
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>, S<1>>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>, S<1>, S<2>>);
```

Feel free to keep (or not) the `int`, `int*` and `int (*)()` ones, I'm not sure how much coverage they add. IMO using `S<n>` conveys more clearly that we're making sure that `dangling` can be constructed from something arbitrary.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105205