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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/arrow.pass.cpp:38-39
+struct WithoutArrowOperator : std::ranges::view_base {
+  struct iterator : contiguous_iterator<Point*> {
+    Point* operator->() const = delete;
+    iterator& operator++();
----------------
Quuxplusone wrote:
> As always, inheriting from test iterators is bad. :) At least copy one of your TODO comments to here: once `contiguous_iterator` no longer has `operator->` this should go away entirely, right? But then you'll need to re-add a test in this vicinity for an iterator type that //does// have `operator->`. Maybe you should preemptively add that test now (it can't hurt) so that you won't forget later and accidentally lose test coverage.
> 
> I haven't looked at the wording for this conditional arrow operator, but surely it's important to test what happens when the underlying iterator type is `int*` (doesn't? have `->`) versus `Widget*` (does? have `->`). Or does `int*` count as having `->` because you can say `p->~TypedefForInt()`?
I refactored this test to remove inheritance and clean it up. It requires D118400 though.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/increment.pass.cpp:88-89
+
+  // Check post-increment for input ranges
+  if constexpr (!std::ranges::forward_range<View>) {
+    std::array<int, 10> array{0, 1, 2, 3, 1, 1, 4, 5, 1, 6};
----------------
Quuxplusone wrote:
> As a general rule, I'd feel safer with hard-coded expectations like
> ```
> template <class Iterator, bool IsOnlyInput>
> constexpr void test() {
>     ~~~
>     if constexpr (IsOnlyInput) {
>         ~~~
>     }
>     if constexpr (!IsOnlyInput) {
>         ~~~
>     }
> }
> ~~~
> test<cpp17_input_iterator<int const*>, true>();
> test<forward_iterator<int const*>, false>();
> ```
> than relying on `std::ranges::forward_range<minimal_view<Iterator, sentinel_wrapper<Iterator>>>` to produce the expected answer. If someone mucks with `minimal_view`, let's fail noisily.
> Also, we're not checking the return value of `it++` on line 95; is that because we expect `std::same_as<decltype(it++), void>` and if so can we assert that?
We are checking that `it++` is the same as `void`, look on current line 99.

I'll hardcode the expectation.


================
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{});
----------------
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).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/base.pass.cpp:37
+
+  FilterSentinel const sent = view.end();
+  std::same_as<Sentinel> auto result = sent.base();
----------------
Quuxplusone wrote:
> `std::same_as<FilterSentinel> auto const sent = view.end();` as long as you're checking things?
I disagree -- that test belongs to the test for `filter_view::end()`, otherwise we're mucking up the fact that we're only testing `.base()` here. I did add these assertions to the tests for `.end()`, they were missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086