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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne 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);
   }
----------------
Quuxplusone wrote:
> (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.
Done both, but I will wait for adding a friend `base()` function to `stride_counting_iterator` because it breaks in weirds ways and this patch is already complicated enough. For now I'll be using `it.base()` instead of `base(it)` to unwrap the `stride_counting_iterator`.


================
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);
----------------
Quuxplusone wrote:
> 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);
> ```
Let's keep this refactoring for a subsequent patch, the same where I can remove `range_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806