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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: Mordante.
Quuxplusone added a comment.
This revision now requires changes to proceed.

FYI I'm still not sold on the benefits of the //stated// goal (removing default-constructibility from `cpp17_input_iterator`), but I like (almost) all the drive-by cleanups that fell out of this.



================
Comment at: libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.addressof.compile.pass.cpp:22
 
-void test() {
+void test(cpp17_input_iterator<std::vector<operator_hijacker>::iterator> i) {
   {
----------------
Can you change this to `cpp17_input_iterator<operator_hijacker*> i` at the same time? I think that's what was intended here: we just need a non-forward iterator whose `value_type` is `operator_hijacker`, and the rest doesn't matter. (Honestly I don't see why it //needs// to be a non-forward iterator — `operator_hijacker *i` might do just as well — but I dunno.)
@mordante, who I believe wrote this code originally: does that sound right to you?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_sentinel.pass.cpp:50
+constexpr void check_assignable_case() {
   auto range = range_t{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+
----------------
Given that `range_t` is hard-coded to `array<int,10>` on line 22, do you have the appetite to change this to a simple
```
int a[10] = {};
```
and then replace `range.begin()` with `a` throughout lines 54–65? Since you're rewriting this test (for the better) anyway...


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_sentinel.pass.cpp:54-55
+    {
+      It first(range.begin());
+      Sent last(It(range.begin() + n));
+      std::ranges::advance(first, last);
----------------
FWIW, here I'd prefer to see
```
auto first = It(range.begin());  // or It(a) after the refactoring above
auto last = Sent(It(range.begin() + n));  // or Sent(It(a + n)) after the refactoring above
```


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_sentinel.pass.cpp:125
 constexpr bool test() {
-  check_assignable_case<cpp17_input_iterator<range_t::const_iterator> >(1);
-  check_assignable_case<forward_iterator<range_t::const_iterator> >(3);
-  check_assignable_case<bidirectional_iterator<range_t::const_iterator> >(4);
-  check_assignable_case<random_access_iterator<range_t::const_iterator> >(5);
-  check_assignable_case<contiguous_iterator<range_t::const_iterator> >(6);
-
-  check_sized_sentinel_case<cpp17_input_iterator<range_t::const_iterator> >(7);
-  check_sized_sentinel_case<cpp20_input_iterator<range_t::const_iterator> >(6);
-  check_sized_sentinel_case<forward_iterator<range_t::const_iterator> >(5);
-  check_sized_sentinel_case<bidirectional_iterator<range_t::const_iterator> >(4);
-  check_sized_sentinel_case<random_access_iterator<range_t::const_iterator> >(3);
-  check_sized_sentinel_case<contiguous_iterator<range_t::const_iterator> >(2);
-
-  check_sentinel_case<cpp17_input_iterator<range_t::const_iterator> >(1);
+  using It = range_t::const_iterator;
+  check_assignable_case<cpp17_input_iterator<It>, sentinel_wrapper<cpp17_input_iterator<It>>>();
----------------
After the `/range/a/` refactoring above, this becomes `using It = int*;` (unless the constness is important for some reason, and I don't think it is), at which point you don't really need the name `It` anymore.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_count_sentinel.pass.cpp:50-64
+  check(cpp17_input_iterator(&range[0]), 1, sentinel_wrapper(cpp17_input_iterator(&range[1])));
   check(forward_iterator(&range[0]), 2, forward_iterator(&range[2]));
   check(bidirectional_iterator(&range[2]), 6, bidirectional_iterator(&range[8]));
   check(random_access_iterator(&range[3]), 2, random_access_iterator(&range[5]));
   check(contiguous_iterator(&range[0]), 5, contiguous_iterator(&range[5]));
 
+  check(cpp17_input_iterator(&range[0]), 0, sentinel_wrapper(cpp17_input_iterator(&range[0])));
----------------
(I note that all of these lines depend on CTAD, which is not thrilling. But that's a pre-existing issue. Just wanted to complain about it.)


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:48-63
+template <bool Count = true, typename It, typename Sent>
+constexpr void check_assignable(It it, Sent last, int const* expected) {
   {
     It result = std::ranges::next(std::move(it), std::move(last));
     assert(&*result == expected);
   }
 
----------------
I believe this change is wrong. IIUC your goal is to get some coverage for sentinels. I'm generally opposed to the current state of your rewrite //only// because it seems to hard-code the assumption that `stride_counting_iterator<X>` is necessarily a sentinel for `stride_counting_iterator<Y>`. I think it needs to use `sentinel_wrapper` instead. It should IMO look like
```
template <std::input_or_output_iterator It>
constexpr void check_assignable(It it, int *last) {
  {
    It result = std::ranges::next(std::move(it), It(last));
    assert(base(result) == last);
  }

  // Count operations
  {
    auto strided_it = stride_counting_iterator<It>(std::move(it));
    auto strided_last = sentinel_wrapper(stride_counting_iterator<It>(It(last)));  // CTAD alert!
    auto result = std::ranges::next(std::move(strided_it), std::move(strided_last));
    assert(result.stride_count() == 0); // because we got here by assigning from last, not by incrementing
    assert(base(result.base()) == last);  // .base() on the strided_iterator, ADL base() on the test iterator itself
  }
}
```
//except// what is this test talking about, "assigning from last"? In general sentinels (e.g. `sentinel_wrapper`) cannot be assigned to their iterator types, only //compared// against them. It sounds like this test file needs an ad-hoc `struct assignable_sentinel` and also a `struct assignable_sized_sentinel`, both of which should trigger https://eel.is/c++draft/iterators#range.iter.op.advance-4.1 .

(I'm totally OK with marking this TODO and coming back to it in a later PR, since the bulk of this PR is a huge improvement.)


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp:76
   // Count operations
-  {
+  if constexpr (Count) {
     auto strided_it = stride_counting_iterator(std::move(it));
----------------
Might be moot if you take my advice above: Could this just be `if constexpr (std::is_copyable_v<It>)`, instead of introducing `bool Count`? Or what is the thing physically preventing this codepath from working for `cpp17_input_iterator`?


================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp:77
 
-    test<cpp17_input_iterator<Base*> >(cpp17_input_iterator<Derived*>(&d));
+    // input iterators are not default constructible, so we don't test that case.
     test<forward_iterator<Base*> >(forward_iterator<Derived*>(&d));
----------------
But this isn't a test for the default constructor — it's a test for the assignment operator. Input iterators must be assignable, right?
Perhaps change line 31 to `std::move_iterator<It> move_ti(It(nullptr));` instead.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:23-26
+  friend forward_iterator<T*> begin(ForwardView&);
+  friend forward_iterator<T*> begin(ForwardView const&);
+  friend sentinel_wrapper<forward_iterator<T*>> end(ForwardView&);
+  friend sentinel_wrapper<forward_iterator<T*>> end(ForwardView const&);
----------------
Any appetite to make this
```
template<class T>
struct ForwardView : std::ranges::view_base {
    forward_iterator<T*> begin() const;
    sentinel_wrapper<forward_iterator<T*>> end() const;
};
```
while you're here? Ditto below. (If that's a bridge too far, then OK, forget it.)


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/common_range.compile.pass.cpp:29-33
+static_assert(std::ranges::common_range<test_common_range<forward_iterator> >);
+static_assert(std::ranges::common_range<test_common_range<forward_iterator> const>);
 
-static_assert(std::ranges::common_range<test_non_const_common_range<cpp17_input_iterator> >);
-static_assert(!std::ranges::common_range<test_non_const_common_range<cpp17_input_iterator> const>);
+static_assert(std::ranges::common_range<test_non_const_common_range<forward_iterator> >);
+static_assert(!std::ranges::common_range<test_non_const_common_range<forward_iterator> const>);
----------------
Here I believe we're losing test coverage of `std::ranges::common_range<R>` where `R` happens to be a common range with input iterators. However, this test is also bad (pre-existing issue). Maybe drop a TODO comment here? `test_common_range` should go away, and this test should look more like
```
template<class It> struct Common { It begin(); It end(); };
template<class It, class Sent> struct NonCommon { It begin(); Sent end(); };

static_assert(std::ranges::common_range<Common<cpp17_input_iterator<int*>>>);
static_assert(std::ranges::common_range<NonCommon<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>>);
static_assert(std::ranges::common_range<Common<cpp20_input_iterator<int*>>>);
static_assert(std::ranges::common_range<NonCommon<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>>);
static_assert(std::ranges::common_range<Common<forward_iterator<int*>>>);
static_assert(std::ranges::common_range<NonCommon<forward_iterator<int*>, sentinel_wrapper<forward_iterator<int*>>>>);
~~~
static_assert(std::ranges::common_range<Common<int*>>);
static_assert(std::ranges::common_range<NonCommon<int*, const int*>>);
static_assert(std::ranges::common_range<NonCommon<int*, sentinel_wrapper<int*>>>);
static_assert(std::ranges::common_range<NonCommon<int*, sized_sentinel<int*>>>);
```
(notice that this includes the `subtly_not_common` case as well)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806