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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb accepted this revision.
cjdb added a comment.

Per offline discussion, I'm okay with this going ahead, provided we return to add `__non_propagating_cache` once I've landed it.



================
Comment at: libcxx/include/__ranges/drop_view.h:133
+    constexpr auto size()
+      requires sized_range<_View>
+    { return __size(*this); }
----------------
Please line up requires-clauses with the start of the function declaration. Small clause (such as this) can go on the same line.

I've usually turned a blind eye to this because comments about formatting are noise that lead to unproductive debates, and my intention has been to get clang-format to take care of this inconsistency in one fell swoop. Having said that, since I don't know when I'll be able to apply the tool project-wide and not get something weird, it's probably worth adopting the "official" libc++ style for future contributors' sakes.


================
Comment at: libcxx/include/__ranges/drop_view.h:141-144
+  template<class _Range>
+  drop_view(_Range&&, range_difference_t<_Range>)
+  // TODO: this is just recreating all_t.
+    -> drop_view<decltype(views::all(std::declval<_Range>()))>;
----------------
Does this need to be fixed?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/general.pass.cpp:61
+template<std::ranges::view View, class T>
+std::ranges::view auto removeBefore(View v, T element) {
+  std::ranges::drop_view out(v, indexOf(v, element) + 1);
----------------
I like that you're using this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037