Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rGb40a3d73dc9c: [libc++] Implement P2446R2 (views::as_rvalue)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__ranges/as_rvalue_view.h | ||
|---|---|---|
| 52 | might be worth testing that if underlying iterator is already a move_iterator, this is calling the move constructor of move_iterator instead of creating a move_iterator<move_iterator<It>> | |
@var-const Can you make a pass at this?
| libcxx/include/__iterator/move_sentinel.h | ||
|---|---|---|
| 53 | Can you please add a simple test that exercises this ctad, even if it is the default-provided one by the compiler? | |
| libcxx/include/__ranges/as_rvalue_view.h | ||
| 62 | Do you have tests for a simple view and a non-simple view? | |
| 75 | Do you have tests for a common range and a non-common range? | |
| 95 | Do you have a test that would fail if you dropped the all_t here? | |
| 104 | This [[nodiscard]] is an extension, so it should be using _LIBCPP_NODISCARD_EXT unless I am mistaken. Please make sure we have a libc++ specific test for this extension. We might be doing it wrong for other adaptors, in which case we should also fix them, but not in this patch. | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp | ||
| 91 | Should we be using the new facilities with meta::for_each here? (everywhere) | |
| libcxx/test/support/test_iterators.h | ||
| 1022 | Any reason why this is not constexpr when it can be? (everywhere) | |
| libcxx/include/__ranges/as_rvalue_view.h | ||
|---|---|---|
| 4 | Please update the <ranges> synopsis as well. | |
| 31 | This needs an include (probably currently relying on the transitive include from all.h). | |
| 111–115 | I think this (an optimization, essentially) deserves a comment. | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp | ||
| 25 | I think this check would be better in test_iterators.h, right after the definition of rvalue_iterator. | |
| 45 | Can you also check piping a container (i.e. something that is not a view) through as_rvalue? Ideally, both lvalue and rvalue. | |
| 49 | Nit: can you add empty lines between the cases? (Here and in some other tests) | |
| 53 | s/auto/decltype(auto)/ (throughout). | |
| 56 | The argument order is swapped in the comment. | |
| 74 | Nit: return 0;. | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/begin.pass.cpp | ||
| 30 | These checks don't seem to be exhaustive. For example, there should probably be a check that !HasBegin if !range<const _View>. | |
| 33 | This only tests the non-const version of begin, right? I think it shouldn't be too hard to also test a const as_rvalue_view here. | |
| 43 | Optional: I think you could wrap this call in a check like if constexpr (std::sentinel_for<Iter, Iter>) and avoid having to call test_range directly from test. | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp | ||
| 59 | s/Begin/End/. | |
| 62 | Can you also test the case where the concept is false? | |
| 71 | Is it possible to just use the std::ranges::common_range concept? | |
| 74 | Same as the begin tests -- I think this should also test when a view is const. | |
| 81 | Same as the begin test -- I think this can be simplified with if constexpr. | |
| 99 | Why are we checking begin here? | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/size.pass.cpp | ||
| 54 | Doesn't seem to be checked. Also, why is it only on the const sized view and not the non-const one? | |
| 57 | Why is the return value only checked for the const case? | |
LGTM (with just a couple of minor comments). I'll leave final approval to @ldionne since he had comments as well.
| libcxx/include/__ranges/as_rvalue_view.h | ||
|---|---|---|
| 111–115 | We can still add a comment, even if this is essentially copied from the Standard. If I'm reading this correctly, this avoids wrapping a range in an rvalue_view if it's already a range of rvalues, to avoid unnecessary bloat. If that's correct, I think it deserves a comment. | |
| libcxx/test/std/ranges/range.adaptors/range.as.rvalue/ctor.pass.cpp | ||
| 39 | Would std::is_convertible work? | |
| libcxx/include/__iterator/move_sentinel.h | ||
|---|---|---|
| 53 | I don't think this was addressed. Please try to ensure that you've addressed feedback before marking a comment as done, otherwise it's really easy to drop (potentially important) comments on the floor. Reviewers usually assume that the feedback has been implemented correctly when the comment is marked as done. | |
| libcxx/include/__ranges/as_rvalue_view.h | ||
| 111–115 | I would agree that no comment is needed since this is indeed what the standard specifies. I'm actually not sure what the comment would say. | |
Can you please add a simple test that exercises this ctad, even if it is the default-provided one by the compiler?