[libcxx-commits] [PATCH] D103341: [libcxx][nfc] Cleanup cpp20_input_iterator.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/support/test_iterators.h:636-637
 {   return !a.operator==(b); }
 
-#ifdef TEST_SUPPORTS_RANGES
-
-// clang-format off
+#if TEST_STD_VER >= 14
 
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Why? This is a move-only iterator, which only makes sense in ranges land.
> It makes sense anywhere it compiles, which is to say >=14.
Yeah, no. This isn't useful in C++14, so it shouldn't be available. Not to mention that it depends on `std::iter_value_t` and `std::iter_difference_t`.


================
Comment at: libcxx/test/support/test_iterators.h:665-676
+  struct sentinel {
+    friend cpp20_input_iterator;
+
+    sentinel() = default;
+    sentinel(I base) : base(base) {}
+
+  private:
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Iterators don't have sentinel sub-types. We have a `sentinel_wrapper` for that.
> Hm. I don't //particularly// dislike `sentinel_wrapper`. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives
> ```
> some_function<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
> some_function<cpp20_input_iterator<int*>, cpp20_input_iterator<int*>::sentinel>();
> ```
> (Right now `sentinel_wrapper` is used only in "range.iter.ops.advance/advance.pass.cpp".)
> 
> I'm okay eliminating this diff for now, pending some examples of how `sentinel{,_wrapper}` would be used in real test cases. Certainly if we're adding `sentinel` we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.
> Hm. I don't //particularly// dislike `sentinel_wrapper`. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives

`sentinel_wrapper` is the sentinel version of our iterator wrappers; if your hard disk has a limited amount of free space where each byte counts, you'd be doing yourself a disservice as we'd need to duplicate this code for each of our other iterators too.

> (Right now sentinel_wrapper is used only in "range.iter.ops.advance/advance.pass.cpp".)

It's also used in `range.iter.ops/range.iter.ops.next`. I should fix up `range.iter.ops/range.iter.ops.prev` to use it too.

> I'm okay eliminating this diff for now, pending some examples of how `sentinel{,_wrapper}` would be used in real test cases. 

If, by "real test cases", you mean "more test cases" (those places where it's used today seem real to me): those should start to flow in when algorithms start to get committed.

> Certainly if we're adding `sentinel` we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.

It's a bit hard to refactor code that doesn't exist yet, but here's an example of what might exist in the near future.

```
auto v = std::vector{1, 2, 3};
std::ranges::find(v.begin(), sentinel_wrapper(v.end()), 0);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103341