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

Fri Oct 7 17:55:01 PDT 2022

var-const accepted this revision.
var-const added a comment.

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

================
Comment at: libcxx/include/__ranges/take_while_view.h:44
+
+// The spec directly uses the unnamed concept inside the member begin and end
+//     constexpr auto begin() const
----------------
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:
```

================
Comment at: libcxx/include/__ranges/take_while_view.h:47
+//       requires range<const V> && indirect_unary_predicate<const Pred, iterator_t<const V>>
+// However, due to a clang-14 and clang-15 bug, the above hard error when `const V` is not a range
+// The workaround is to create a named concept and use the concept instead.
----------------
Nit: missing a full stop.

================
Comment at: libcxx/include/__ranges/take_while_view.h:47
+//       requires range<const V> && indirect_unary_predicate<const Pred, iterator_t<const V>>
+// However, due to a clang-14 and clang-15 bug, the above hard error when `const V` is not a range
+// The workaround is to create a named concept and use the concept instead.
----------------
var-const wrote:
> Nit: missing a full stop.
Nit: `hard errors` or `produces a hard error`.

================
Comment at: libcxx/include/__ranges/take_while_view.h:50
+// As of take_while_view is implemented, the clang-trunk has already fixed the bug.
+// It is OK to remove the workaround once our CI no longer uses clang-14, clang-15 based compilers.
+// because we don't actually expect a lot of vendors to ship a new libc++ with an old clang
----------------
Nit: `s/./,/`.

================
Comment at: libcxx/include/__ranges/take_while_view.h:51
+// It is OK to remove the workaround once our CI no longer uses clang-14, clang-15 based compilers.
+// because we don't actually expect a lot of vendors to ship a new libc++ with an old clang
+template <class _View, class _Pred>
----------------
Nit: missing full stop.

================
Comment at: libcxx/include/__ranges/take_while_view.h:59
+class take_while_view : public view_interface<take_while_view<_View, _Pred>> {
+  // [range.take.while.sentinel], class template take_while_view::sentinel
+  template <bool>
----------------
Nit: does this comment actually add value?

================
Comment at: libcxx/include/__ranges/take_while_view.h:99
+  {
+    return __sentinel<false>(ranges::end(__base_), std::addressof(*__pred_));
+  }
----------------
Optional: consider adding a comment to make it clear what `false` stands for, like `__sentinel</*_Const=*/false>` (similar suggestion below).

================
Comment at: libcxx/include/__ranges/take_while_view.h:155
+          -> decltype(/*--*/ take_while_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred))) {
+    return /*-------------*/ take_while_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred));
+  }
----------------
Question: this is a creative way to prevent `clang-format` from breaking the manual indentation, right?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp:63
+    using Result                     = std::ranges::take_while_view<MoveOnlyView, Pred>;
+    std::same_as<Result> auto result = std::views::take_while(Pred{})(MoveOnlyView{buff});
+    auto expected                    = {1, 2};
----------------
Nit: `decltype(auto)`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/begin.pass.cpp:91
+
+  // NotPredForConst LWG 3450
+  {
----------------
Can you add a little more context to the comment so that it's not necessary to open the LWG issue?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp:37
+// clang-format off
+static_assert( std::is_default_constructible_v<std::ranges::take_while_view<View<true >, Pred<true >>>);
+static_assert(!std::is_default_constructible_v<std::ranges::take_while_view<View<false>, Pred<true >>>);
----------------
Can these be runtime checks?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.default.pass.cpp:37
+// clang-format off
+static_assert( std::is_default_constructible_v<std::ranges::take_while_view<View<true >, Pred<true >>>);
+static_assert(!std::is_default_constructible_v<std::ranges::take_while_view<View<false>, Pred<true >>>);
----------------
var-const wrote:
> Can these be runtime checks?
Optional: I'd use the `/*defaultInitable=*/` comments to make the booleans clearer.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/end.pass.cpp:93
+
+  // NotPredForConst LWG 3450
+  {
----------------
Same nit here.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:68
+
+struct Pred {
+  constexpr bool operator()(int i) const { return i < 3; }
----------------
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)

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:76
+
+static_assert(HasEqual<std::ranges::iterator_t<std::ranges::take_while_view<R, Pred>>,
+                       std::ranges::sentinel_t<std::ranges::take_while_view<R, Pred>>>);
----------------
Very optional: it looks like a few `using` statements would cut down a lot of line noise (i.e., the `std::ranges::` repetitions).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:128
+
+  // base.begin == base.end
+  {
----------------
Nit: I'd move this case up so that the two cases where `begin != end` are next to each other.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:128
+
+  // base.begin == base.end
+  {
----------------
var-const wrote:
> Nit: I'd move this case up so that the two cases where `begin != end` are next to each other.
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.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:147
+    auto st   = getEnd(twv);
+    ++iter;
+    ++iter;
----------------
Nit: check that `iter != sent` before incrementing.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:150
+    assert(iter == st);
+  }
+}
----------------
Can you also check an empty range?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/types.h:30
+
+struct SimpleView : IntBufferView {
+  using IntBufferView::IntBufferView;
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134952