Details
- Reviewers
cjdb ldionne - Group Reviewers
Restricted Project - Commits
- rG481ad59b9fa4: [libcxx][ranges] Add `std::ranges::single_view`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__ranges/copyable_box.h | ||
---|---|---|
170 | _VSTD::addressof(__val_) Also, would it be possible to add a test for it, like we do for operator*? | |
libcxx/include/__ranges/single_view.h | ||
2 | Could you add views::single at the same time? It's pretty small so it makes sense to implement both in the same go, I think. Also, this makes me think that we don't have views::empty, right? | |
41 | Here you could use __value_(std::in_place, __t) instead, and you would not have to modify __copyable_box. Same for the rvalue ref below. | |
libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp | ||
14 | Since the spec documents this as being static, can you add a test that std::ranges::single_view<int>::size() works as well? |
libcxx/include/__ranges/single_view.h | ||
---|---|---|
2 | If you're going to take @ldionne up on this (and I think you should), please DM me so we can discuss a cohesive design for the adaptor closures. I don't want us to have wildly different ideas about what goes in this namespace. | |
libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp | ||
14 | Unless there are already tests in ranges::size for static members, is it worth adding a test here for ranges::size too? |
libcxx/test/std/ranges/range.factories/range.single.view/view_conformance.compile.pass.cpp | ||
---|---|---|
25 ↗ | (On Diff #362066) | 🤦♂️ |
LGTM with all comments applied and CI passing.
libcxx/include/__ranges/single_view.h | ||
---|---|---|
2 | views::single is not a range adaptor object, so we still are not implementing the pipe operator. But regardless, let's chat on Discord since you seem to have a specific approach in mind for the pipe operator. | |
libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp | ||
2 | Stray comment? | |
11 | I really like that you're testing this property of the type, thanks for being thorough. | |
libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp | ||
14 | I'm not sure I understand your comment. Are you asking that we should add a test for ranges::size() on a type that provides a static size() method? If that's your suggestion, I would agree. It's actually a hole in our current testing coverage, since the spec for ranges::size() basically says "use x.size() if that is valid", which does not specify whether .size() is a static or non-static member function. So as a fly-by fix in this patch, @zoecarver can you add a test in ranges::size() with a type that defines a static size() function? |
So it appears that the assign test crashes on GCC trunk! Anyway, I filed a bug (id = 101663), and I'll mark this test as unsupported on GCC.
libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp | ||
---|---|---|
11 | Thank you :) |
LGTM pending last bit of feedback (just test it locally, no need to wait for CI).
libcxx/test/std/ranges/range.factories/range.single.view/range_concept_conformance.compile.pass.cpp | ||
---|---|---|
25 | This should be input_range. |
_VSTD::addressof(__val_)
Also, would it be possible to add a test for it, like we do for operator*?