Page MenuHomePhabricator

[libc++][ranges]implement `std::views::take_while`
ClosedPublic

Authored by huixie90 on Sep 30 2022, 5:07 AM.

Details

Diff Detail

Event Timeline

huixie90 created this revision.Sep 30 2022, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 5:07 AM
huixie90 requested review of this revision.Sep 30 2022, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 5:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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?

huixie90 updated this revision to Diff 465559.Oct 5 2022, 2:37 PM
huixie90 marked 20 inline comments as done.

address feedback

huixie90 updated this revision to Diff 465664.Oct 5 2022, 11:55 PM

CI

libcxx/docs/Status/Cxx2bIssues.csv
1

I did a survey in this patch
https://reviews.llvm.org/D133317?vs=458020&id=458045

Basically there are 4 views left in the paper
istream_view (in progress)
take_while_view(in progress)
drop_while_view(to do)
elements_view (to do)

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)

philnik accepted this revision as: philnik.Oct 6 2022, 3:00 AM

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?

ldionne accepted this revision.Oct 6 2022, 10:03 AM
ldionne added inline comments.
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
This revision is now accepted and ready to land.Oct 6 2022, 10:03 AM

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).

huixie90 updated this revision to Diff 465978.Oct 7 2022, 12:04 AM
huixie90 marked 6 inline comments as done.

address feedback

var-const accepted this revision.Oct 7 2022, 5:54 PM

LGTM -- just some nitpicks. Thanks a lot for working on this!

libcxx/include/__ranges/take_while_view.h
45

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:
48

Nit: missing a full stop.

48

Nit: hard errors or produces a hard error.

51

Nit: s/./,/.

52

Nit: missing full stop.

60

Nit: does this comment actually add value?

100

Optional: consider adding a comment to make it clear what false stands for, like __sentinel</*_Const=*/false> (similar suggestion below).

156

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
64

Nit: decltype(auto).

libcxx/test/std/ranges/range.adaptors/range.take.while/begin.pass.cpp
92

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
38

Can these be runtime checks?

38

Optional: I'd use the /*defaultInitable=*/ comments to make the booleans clearer.

libcxx/test/std/ranges/range.adaptors/range.take.while/end.pass.cpp
94

Same nit here.

libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp
69

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)

77

Very optional: it looks like a few using statements would cut down a lot of line noise (i.e., the std::ranges:: repetitions).

129

Nit: I'd move this case up so that the two cases where begin != end are next to each other.

129

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.

148

Nit: check that iter != sent before incrementing.

151

Can you also check an empty range?

libcxx/test/std/ranges/range.adaptors/range.take.while/types.h
31

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.

huixie90 updated this revision to Diff 466287.Oct 8 2022, 8:20 AM
huixie90 marked 19 inline comments as done.

address feedback and rebase

libcxx/include/__ranges/take_while_view.h
156

@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
38

The runtime tests are right below these static_assert

libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp
129

You are right. all these comments are wrong. I updated these comments now

This revision was automatically updated to reflect the committed changes.