[libcxx-commits] [PATCH] D115806: [libc++] Remove incorrect default constructor in cpp17_input_iterator

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:50-53
   {
-    It result = std::ranges::next(std::move(it), std::move(last));
+    It result = std::ranges::next(It(it), Sent(It(last)));
     assert(&*result == expected);
   }
----------------
(1, throughout) `&*result` would be better spelled `base(result)`.

(2) Lines 50–53 now seem pointless, or at least unrelated to the "assignable" aspect of this function. Vice versa, lines 56–62 are checking that the assignment happens, but they're no longer checking the It-Sent case: they're checking only the It-It case.
Per my old comment below, I can't think of any good way to fix this except to introduce a new `assignable_sentinel` type in this file.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:92-93
 
-template <std::input_or_output_iterator It>
-constexpr void check_sentinel(It it, It last, int const* expected) {
-  auto n = (last.base() - it.base());
+template <bool Count, typename It>
+constexpr void check_sentinel(int* it, int* last, int const* expected) {
+  auto n = (last - it);
----------------
This could be simplified to
```
template <bool Count, class It>
constexpr void check_sentinel(It it, int n) {
  It last = It(base(it) + n);
  {
    auto sent = sentinel_wrapper<It>(last);
    It result = std::ranges::next(it, sent);
    assert(base(result) == base(last));
  }
```
and so on. This would vastly simplify the callers, I think, e.g.
```
check_sentinel<true,  forward_iterator<int*>>(      &range[0], &range[3], &range[3]);
```
becomes
```
check_sentinel<true>(forward_iterator<int*>(range), 3);
```


================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op.const/default.pass.cpp:32
 {
-    test<cpp17_input_iterator<char*> >();
+    // input iterators are not required to be default constructible, so not testing that case
     test<forward_iterator<char*> >();
----------------
"There's no such thing as an archetype" alert! :) This comment is describing lack of test coverage, so it would be better worded as
```
// we no longer have a test iterator that is both input and default-constructible, so not testing that case
```
You could use something like `test<std::istream_iterator<int>>();` here, to avoid losing test coverage, but that might require dragging in `<istream>` to get it to compile. Really I'm just carping about the excuse-making wording of the proposed comment. It's not like it's //C++'s// fault that we lost this coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806