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

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

Quuxplusone added a comment.

Somewhat but not entirely reviewed.



================
Comment at: libcxx/include/__ranges/filter_view.h:124-125
+    using iterator_category =
+      _If<derived_from<bidirectional_iterator_tag, _Cat>, bidirectional_iterator_tag,
+      _If<derived_from<forward_iterator_tag, _Cat>,       forward_iterator_tag,
+      /* else */                                          _Cat
----------------
ldionne wrote:
> Quuxplusone wrote:
> > These conditions smell backwards. Shouldn't it be `_Cat satisfies derived_from<foo_tag>`, i.e. `derived_from<_Cat, foo_tag>`?
> > But actually I would suggest testing the full concept: `_If<bidirectional_iterator<_Cat>, bidirectional_iterator_tag, ...`. Because (sadly) it's perfectly possible in C++20 to have an iterator advertising `bidirectional_iterator_tag` that itself is only a `forward_iterator`. The `forward_iterator` concept does not have any "penalty for overpromising."
> > 
> > Both the bug(?) and the sad scenario could use a test case.
> Thanks for noticing, the conditions were indeed backwards. I added tests for the iterator in the latest revision and they cover that bug.
> 
> > But actually I would suggest testing the full concept:
> 
> Regarding that, I'm really sticking to http://eel.is/c++draft/range.filter#iterator-3, which uses `derived_from` explicitly. Actually, while we're talking about that, do you see any way we can ever hit the `else` part (http://eel.is/c++draft/range.filter#iterator-3.4)? I don't. The iterator *must* be a `forward_iterator` at least (because `View` is a `forward_range`), which means that its `iterator_category` will be at least `forward_iterator_tag`. This means that it's always going to at least use line 125, and never go into the `else`.
`forward_iterator<C>` implies `derived_from<ITER-CONCEPT(C), forward_iterator_tag>`, but (sadly, as usual) that isn't the same thing as `derived_from<iterator_traits<C>::iterator_category, forward_iterator_tag>`. For example: https://godbolt.org/z/ExzeoEbjv


================
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++();
----------------
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()`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/decrement.pass.cpp:116
+  test<random_access_iterator<int const*>>();
+  test<contiguous_iterator<int const*>>();
+
----------------
Also `test<int*>()`? Likewise throughout.


================
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};
----------------
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?


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


================
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();
----------------
`std::same_as<FilterSentinel> auto const sent = view.end();` as long as you're checking things?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086