[libcxx-commits] [PATCH] D109086: [libc++] Implement ranges::filter_view

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp:13
+
+// std::views::filter
+
----------------
Please `git grep views::filter` and uncomment the existing line you find, too.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/base.pass.cpp:29-35
+  auto make_filter_view = [](auto begin, auto end, auto pred) {
+    View view{Iterator(begin), Sentinel(Iterator(end))};
+    return FilterView(std::move(view), pred);
+  };
+
+  std::array<int, 5> array{0, 1, 2, 3, 4};
+  FilterView view = make_filter_view(array.begin(), array.end(), AlwaysTrue{});
----------------
ldionne wrote:
> Quuxplusone wrote:
> > At least the `auto`s on line 29 could be `int*`s. But I'd write this as
> > ```
> > // line 23: rename Iterator to It, and eliminate Sentinel
> > int a[5] = {};
> > auto view = FilterView(
> >     View(It(a), sentinel_wrapper<It>(It(a+5))),
> >     AlwaysTrue{}
> > );
> > ```
> I was going to say this: I'll use `int*` instead of `auto`, however I'll keep the rest as-is. This is used consistently throughout the patch, and I'd like to avoid doing it differently in even a single place.
> 
> But then I looked and I need to use `auto` because `array`'s iterators are not portably pointers. And I don't want to get rid of `std::array` throughout these tests because it makes my life easier (can do `.begin()` and `.end()` instead of more complicated alternatives).
> And I don't want to get rid of std::array throughout these tests because it makes my life easier (can do .begin() and .end() instead of more complicated alternatives).

But `a, a+5` is //less// complicated than `a.begin(), a.end()`! It just seems like you could get rid of a lot of the cruft here if you wanted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086