[libcxx-commits] [PATCH] D134952: [libc++][ranges]implement `std::views::take_while`

Mon Oct 3 02:22:50 PDT 2022

philnik added a comment.

Looks pretty good so far. The only major thing is the test which looks wrong.

================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:1
 "Issue #","Issue Name","Meeting","Status","First released version","Labels"
 "`2839 <https://wg21.link/LWG2839>`__","Self-move-assignment of library types, again","November 2020","|Nothing To Do|",""
----------------
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`.

================
Comment at: libcxx/include/__ranges/take_while_view.h:9
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___RANGES_TAKE_WHILE_VIEW_H
+#define _LIBCPP___RANGES_TAKE_WHILE_VIEW_H
----------------

================
Comment at: libcxx/include/__ranges/take_while_view.h:51
+  _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
+  _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
+
----------------
The standard calls this `movable-box`. Has the standard renamed it at some point?

================
Comment at: libcxx/include/__ranges/take_while_view.h:96-97
+
+template <class _Rng, class _Pred>
+take_while_view(_Rng&&, _Pred) -> take_while_view<views::all_t<_Rng>, _Pred>;
+
----------------
I read rng normally as random-number-generator, not range. It also looks like we don't use `_Rng` anywhere else.

================
Comment at: libcxx/include/__ranges/take_while_view.h:138-145
+  // clang-format off
+    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)); }
----------------
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).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
----------------
Why is this test marked `UNSUPPORTED: c++20`? Same question for some of the other tests.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/base.pass.cpp:46
+constexpr bool test() {
+  // const &
+  {
----------------
Optional: Maybe also check `&` and `const &&`

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp:18
+template <bool defaultInitable>
+struct V : std::ranges::view_interface<V<defaultInitable>> {
+  int i;
----------------

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp:20
+  int i;
+  constexpr V()
+    requires defaultInitable
----------------
I think this should be `explicit` to check that the implementation doesn't do something like `View base = {}`. Same for the predicate.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp:43
+  {
+    std::ranges::take_while_view<V<true>, Pred<true>> twv;
+    assert(twv.base().i == 0);
----------------
To check that the ctor isn't explicit.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp:37
+  {
+    std::ranges::take_while_view<V, Pred> twv(V{{}, MoveOnly{5}}, Pred{});
+    assert(twv.pred().moved);
----------------
To check that the ctor isn't explicit.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/pred.pass.cpp:29
+constexpr bool test() {
+  {
+    std::ranges::take_while_view<View, Pred> twv{{}, Pred{5}};
----------------
Again, optional: maybe also check `&`, `&&` and `const &&`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/pred.pass.cpp:30
+  {
+    std::ranges::take_while_view<View, Pred> twv{{}, Pred{5}};
+    decltype(auto) x = twv.pred();
----------------

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/range.concept.compile.pass.cpp:23
+template <class It>
+using R = std::ranges::subrange<It, sentinel_wrapper<It>>;
+
----------------

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/range.concept.compile.pass.cpp:47-48
+static_assert(!HasTakeWhileView<R<int*>, int>);
+struct Foo {};
+static_assert(!HasTakeWhileView<R<Foo*>, Pred<int>>);
----------------
Maybe just use `int**` and `Pred<int>`?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.base.pass.cpp:70-74
+    int i     = 10;
+    int* iter = &i;
+    Sentinel s(Sent{0}, &pred);
+
+    [[maybe_unused]] bool b = iter == s;
----------------
`i`, `iter` and `b` seem to do nothing here, or am I missing something?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.convert.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This test looks wrong. You are claiming to test `sentinel(sentinel<!Const>)`, but AFAICT you test `explicit sentinel(sentinel_t<Base>, const Pred*)`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.default.pass.cpp:32

     Sentinel s1;
+    assert(!s1.base().b);
----------------
explicit test

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:118
+
+  // non const, base.begin != base.end && !pred(i)
+  {
----------------
I guess the `non const` part of the comments is from a previous iteration?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:124
+    auto iter = getBegin(twv);
+    auto st   = getEnd(twv);
+    assert(iter != st);
----------------

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
Did you edit this file accidentally, so is Phabricator just showing a weird diff?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134952/new/

https://reviews.llvm.org/D134952