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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
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));
+  }
----------------
var-const wrote:
> Question: this is a creative way to prevent `clang-format` from breaking the manual indentation, right?
@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.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/base.pass.cpp:2
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
ldionne wrote:
> 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`.
Confirmed with @var-const , we don't have any "robust" tests for views, only for algorithms. 


================
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:
> var-const wrote:
> > Can these be runtime checks?
> Optional: I'd use the `/*defaultInitable=*/` comments to make the booleans clearer.
The runtime tests are right below these static_assert


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/equality.pass.cpp:128
+
+  // base.begin == base.end
+  {
----------------
var-const wrote:
> 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.
You are right. all these comments are wrong. I updated these comments now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134952