Details
- Reviewers
var-const philnik ldionne - Group Reviewers
Restricted Project - Commits
- rGa2c6a1193f41: [libc++][ranges]implement `std::views::take_while`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks pretty good so far. The only major thing is the test which looks wrong.
| libcxx/docs/Status/Cxx2bIssues.csv | ||
|---|---|---|
| 1 | Could you maybe do a survey of what we have to still implement for P1035R7? It's one of the really large papers, so it would be nice to document what we have already implemented. Skimming through the paper it looks like we're only missing elements_view and take_while_view. | |
| libcxx/include/__ranges/take_while_view.h | ||
| 10 | ||
| 52 | The standard calls this movable-box. Has the standard renamed it at some point? | |
| 97–98 | I read rng normally as random-number-generator, not range. It also looks like we don't use _Rng anywhere else. | |
| 139–146 | Just a suggestion: Maybe use /*---*/ to pad out the difference. It would look like this: struct __fn {
template <class _Range, class _Pred>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Pred&& __pred) const
noexcept(noexcept(/**/ take_while_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred))))
-> decltype(/*--*/ take_while_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred))) {
return /*-------------*/ take_while_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred));
}
template <class _Pred>
requires constructible_from<decay_t<_Pred>, _Pred>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const
noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) {
return __range_adaptor_closure_t(std::__bind_back(*this, std::forward<_Pred>(__pred)));
}
};It's not perfect, but IMO it's better than manually formatting it (the indentation is wrong currently). | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp | ||
| 10 | Why is this test marked UNSUPPORTED: c++20? Same question for some of the other tests. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/base.pass.cpp | ||
| 47 | Optional: Maybe also check & and const && | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp | ||
| 19 | ||
| 21 | I think this should be explicit to check that the implementation doesn't do something like View base = {}. Same for the predicate. | |
| 44 | To check that the ctor isn't explicit. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp | ||
| 38 | To check that the ctor isn't explicit. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/pred.pass.cpp | ||
| 30 | Again, optional: maybe also check &, && and const &&. | |
| 31 | ||
| libcxx/test/std/ranges/range.adaptors/range.take.while/range.concept.compile.pass.cpp | ||
| 24 | ||
| 48–49 | Maybe just use int** and Pred<int>? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.base.pass.cpp | ||
| 71–75 | i, iter and b seem to do nothing here, or am I missing something? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.convert.pass.cpp | ||
| 2 | This test looks wrong. You are claiming to test sentinel(sentinel<!Const>), but AFAICT you test explicit sentinel(sentinel_t<Base>, const Pred*). | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.default.pass.cpp | ||
| 32 | explicit test | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp | ||
| 119 | I guess the non const part of the comments is from a previous iteration? | |
| 125 | ||
| libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp | ||
| 1 ↗ | (On Diff #464223) | Did you edit this file accidentally, so is Phabricator just showing a weird diff? |
CI
| libcxx/docs/Status/Cxx2bIssues.csv | ||
|---|---|---|
| 1 | I did a survey in this patch Basically there are 4 views left in the paper Louis pointed out that the remaining work in this paper isn't huge and we probably don't need to break it up in more csvs | |
| libcxx/include/__ranges/take_while_view.h | ||
| 52 | copyable-box is replaced by movable-box in P2494R2, which we haven't implemented yet | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.base.pass.cpp | ||
| 71–75 | I'd like to test that the constructor correctly initialise the member predicate pointer to the one passed in. The only observable way is to use ==. Here I simply trigger the == and assert that the pred is called | |
| libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp | ||
| 1 ↗ | (On Diff #464223) | ops. I used clangd's rename in one test and it somehow renamed the class also in this file. (clangd is technically correct. It just wasn't aware that our tests are completely different programme. Otherwise these types defined in the tests would refer to the same symbol as all of them have external linkage) |
LGTM % nits.
| libcxx/docs/Status/Cxx2bIssues.csv | ||
|---|---|---|
| 1 | We don't need a csv, but marking the paper as in progress and adding a note that says that we have to implement drop_while_view and elements_view would be nice. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.base.pass.cpp | ||
| 71–75 | The comparison should be true, right? Could you assert that? | |
| libcxx/docs/Status/Cxx2bIssues.csv | ||
|---|---|---|
| 37 | I can't find a test that explicitly calls out this LWG issue. Are we missing it? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/base.pass.cpp | ||
| 3 | Not attached to this file: @var-const , did we have general tests that we needed to update whenever adding a new view? We definitely had those for ranges algorithms, IIRC it was checking some properties of CPOs, but I don't remember the details. A bunch of them are called robust_against_FOO. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp | ||
| 37 | Suggestion to align these lines (and also the View and the Preds) so we can see more easily that you are just flipping between true and false. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/general.pass.cpp | ||
| 12 | ||
FWIW, I would be okay with marking the tests as XFAIL: clang-14, clang-15 unless the workaround in take_while_view for that compiler bug is really simple.
The reason why I'm so keen to do this is that our support for older clangs is mainly to make sure that people's CI systems keep working fine as we make changes, but we don't actually expect a lot of vendors to ship a new libc++ with an old clang. And users of such a setup that end up using take_while_view should be even more rare. Anyway, I'm fine either way, but this is just the justification for why I'm happy with marking the test as XFAIL if the workaround introduces a lot of tech debt in libc++ (which we will likely not remove in the future unless it's clearly marked as such).
LGTM -- just some nitpicks. Thanks a lot for working on this!
| libcxx/include/__ranges/take_while_view.h | ||
|---|---|---|
| 44 | Nit: I think the right term is requirement? I think it would be a little clearer as something like: The spec uses an unnamed requirement inside the `begin` and `end` member functions: | |
| 47 | Nit: missing a full stop. | |
| 47 | Nit: hard errors or produces a hard error. | |
| 50 | Nit: s/./,/. | |
| 51 | Nit: missing full stop. | |
| 59 | Nit: does this comment actually add value? | |
| 99 | Optional: consider adding a comment to make it clear what false stands for, like __sentinel</*_Const=*/false> (similar suggestion below). | |
| 155 | Question: this is a creative way to prevent clang-format from breaking the manual indentation, right? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp | ||
| 63 | Nit: decltype(auto). | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/begin.pass.cpp | ||
| 91 | Can you add a little more context to the comment so that it's not necessary to open the LWG issue? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp | ||
| 37 | Can these be runtime checks? | |
| 37 | Optional: I'd use the /*defaultInitable=*/ comments to make the booleans clearer. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/end.pass.cpp | ||
| 93 | Same nit here. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp | ||
| 68 | Can you rename the pred so that it's obvious what it does without having to look up the definition? It makes reading test cases below easier. (I usually use IsNeg and IsOdd/IsEven since they seem a little more generic than comparing with an arbitrary number, but something like LessThan3 is okay too) | |
| 76 | Very optional: it looks like a few using statements would cut down a lot of line noise (i.e., the std::ranges:: repetitions). | |
| 128 | Nit: I'd move this case up so that the two cases where begin != end are next to each other. | |
| 128 | Perhaps I'm missing something, but it's not begin that is equal to end, right? Rather, it's a sequence that doesn't have an element satisfying the predicate. | |
| 147 | Nit: check that iter != sent before incrementing. | |
| 150 | Can you also check an empty range? | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/types.h | ||
| 30 | Can you static_assert that these views have the desired properties? (Also serves as a form of documentation) You can use the libc++-specific macro for accessing internal concept names. | |
address feedback and rebase
| libcxx/include/__ranges/take_while_view.h | ||
|---|---|---|
| 155 | @philnik suggested it. It is a way to prevent clang-format breaking the manual indentation that keeps the same 3 expressions aligned, and at the same time, formats other stuff properly. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/base.pass.cpp | ||
| 3 | Confirmed with @var-const , we don't have any "robust" tests for views, only for algorithms. | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp | ||
| 37 | The runtime tests are right below these static_assert | |
| libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp | ||
| 128 | You are right. all these comments are wrong. I updated these comments now | |
Could you maybe do a survey of what we have to still implement for P1035R7? It's one of the really large papers, so it would be nice to document what we have already implemented. Skimming through the paper it looks like we're only missing elements_view and take_while_view.