[libcxx-commits] [PATCH] D102037: [libcxx][views] Add drop_view.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.

- Some of my comments (especially about splitting tests) also apply to other in-flight reviews like `transform_view`.
- Please don't forget to mark comments as done when they're not relevant anymore (whether they have been implemented or you've decided you wouldn't do it), so it's easy to keep track of things.

I'll wait for this to be rebased onto `non_propagating_cache` before I re-review the implementation.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please split this into more files as it makes sense. Each file tests one thing, and what's being tested is at the top of level in a comment, as we always do.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/range.drop.view.pass.cpp:23
+
+int globalBuff[8];
+
----------------
cjdb wrote:
> Nit: I'd prefer if this were `std::array<int, 8>`.
I'll actually disagree here - I think it's good to stay minimal when we don't lose anything doing so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037