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

Thu Dec 30 13:08:17 PST 2021

ldionne marked an inline comment as done.
ldionne added inline comments.

================
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
----------------
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`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086