- Implements part of P0896 'The One Ranges Proposal'
- Implements http://wg21.link/range.dangling
Details
- Reviewers
ldionne zoecarver Mordante - Group Reviewers
Restricted Project - Commits
- rG74fd3cb8cd3e: [libcxx][ranges] implements dangling, borrowed_iterator_t, borrowed_subrange_t
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__ranges/borrowed_subrange_t.h | ||
---|---|---|
37–38 ↗ | (On Diff #355600) | I'm under the impression that libc++ has a "better conditional_t" somewhere that doesn't require one class template instantiation per type? |
libcxx/include/__ranges/borrowed_subrange_t.h | ||
---|---|---|
37–38 ↗ | (On Diff #355600) | You can either use _If or just plain conditional_t. _If creates fewer instantiations. |
LGTM mod comments!
libcxx/include/__ranges/borrowed_subrange_t.h | ||
---|---|---|
37–38 ↗ | (On Diff #355600) | 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) template<range _Rp> using borrowed_subrange_t = _If<borrowed_range<_Rp>, subrange<iterator_t<_Rp>>, dangling>; |
libcxx/include/__ranges/dangling.h | ||
35–42 | template<range _Rp> using borrowed_subrange_t = _If<borrowed_range<_Rp>, iterator_t<_Rp>, dangling>; | |
libcxx/include/__ranges/subrange.h | ||
20–21 | Yikes. I guess I need to audit graph_header_deps.py; it should have caught even this trivially circular inclusion. | |
libcxx/include/ranges | ||
141 | a-z plz (and also in the modulemap and CMakeLists) | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
31 | 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 :)). | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_subrange.compile.pass.cpp | ||
24 | 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. | |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
27–28 | 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. |
- replaces specialisations with _If
- adds tests
- moves borrowed_subrange_t into __range/subrange.h (ATTN @ldionne)
A few nits. But LGTM.
libcxx/include/ranges | ||
---|---|---|
98 | Thanks. | |
101 | Is this implemented? It doesn't look like it, yet. | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
33 | 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). |
libcxx/include/__ranges/borrowed_subrange_t.h | ||
---|---|---|
37–38 ↗ | (On Diff #355600) | Fewer instantiations is always good. |
libcxx/include/ranges | ||
101 | This was the first thing implemented in <ranges> (see line 136). | |
141 | Done, done, and done! | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
31 | has_X is a trait, not a concept :) | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_subrange.compile.pass.cpp | ||
24 |
Yes, good idea (also applied to borrowed_iterator).
I'd still prefer to keep the other types to make sure borrowed_X isn't hard-coded to a single type. | |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
27–28 |
Keeping lines 25-7 (making sure this works with user-defined types is important), and added both your lines.
🤷 The constructor is variadic, so it's probably intentional and mandated? |
libcxx/include/__ranges/dangling.h | ||
---|---|---|
33 | _LIBCPP_HIDE_FROM_ABI | |
libcxx/include/__ranges/subrange.h | ||
22 | You need <type_traits> for _If (here and in dangling.h). | |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
33 | 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? | |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
27–28 | 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. |
applies requested changes
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
---|---|---|
33 | Renamed, but keeping as constexpr bool (this is not a concept). |
@DavidSpickett I see the Linaro bots have started failing when building libunwind. I looked into it a little bit, but I didn't see any recent changes in libunwind. Did you make changes to your bots?
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
---|---|---|
39–43 | I think this belongs to dangling.compile.pass.cpp. | |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
2 | This should be a .pass.cpp test, and we should test that we can actually run (not only compile) code that calls the variadic constructor (and the default constructor), since it's defined with an empty body in the Standard. As it stands, I think our tests would pass if we had not defined the constructor at all and only declared it, but we would be non-conforming. |
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp | ||
---|---|---|
39–43 |
This is where this comment belongs (for some reason it doesn't show up). |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
---|---|---|
2 | Does it really need to be a .pass.cpp? Surely a bunch of constexpr objects should do the trick, no? |
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp | ||
---|---|---|
2 | We really want to test exactly what the users are going to do, and that includes "running" that empty constructor at runtime. Even though in practice we do know it will be optimized out and all, it's still what we need to check. |
Applies @ldionne's feedback about adding a run-time test.
Thought I already did this, but apparently not. Explains why there hasn't been any further progress on this.
_LIBCPP_HIDE_FROM_ABI