[libcxx-commits] [PATCH] D135460: [libc++][ranges] implement `std::ranges::drop_while_view`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__ranges/drop_while_view.h:68
+  _LIBCPP_HIDE_FROM_ABI constexpr auto begin() {
+    _LIBCPP_ASSERT(__pred_.__has_value(), "predicate must contain a value");
+    if constexpr (_UseCache) {
----------------
Maybe that's a bit better for a user? Per live review, we think that's the only situation that can lead us to this assertion failing.

Also, this is non-blocking because extremely pedantic, but it might be nice to add an `assert.<xxxxxx>.pass.cpp` test to check that behavior. You can take inspiration of other tests like `libcxx/test/libcxx/strings/basic.string/string.access/assert.back.pass.cpp`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop.while/ctor.default.pass.cpp:17
+
+template <bool defaultInitable>
+struct View : std::ranges::view_base {
----------------
Same below.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop.while/ctor.default.pass.cpp:19-22
+  int i;
+  constexpr explicit View()
+    requires defaultInitable
+  = default;
----------------
Maybe you could initialize `i` to a non-zero value just to rule out the fact that we're accessing zeroed-by-chance memory that would be uninitialized below?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop.while/general.pass.cpp:20
+int main(int, char**) {
+  constexpr decltype(auto) source = "  \t   \t   \t   hello there";
+  auto is_invisible     = [](const auto x) { return x == ' ' || x == '\t'; };
----------------
Maybe that's closer to what a user would actually write?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135460