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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 6 inline comments as done.
ldionne added a comment.

In D115806#3212758 <https://reviews.llvm.org/D115806#3212758>, @Quuxplusone wrote:

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

Well, if `cpp17_input_iterator` is default constructible, then it is not an archetype for `Cpp17InputIterator` anymore. And being an archetype for `Cpp17InputIterator` is the very reason for that type's existence.



================
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) {
   {
----------------
Quuxplusone wrote:
> 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?
Yeah, I think that makes sense.


================
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};
+
----------------
Quuxplusone wrote:
> 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...
I suggest doing that in a separate PR and changing all the tests under `range.iter.ops` if we really want to do it. Otherwise, I'll just be introducing inconsistency with the neighboring tests.


================
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);
----------------
Quuxplusone wrote:
> 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
> ```
What criteria do you use for determining `auto` vs classic variable declarations? Why are you fine with e.g. the `stride_counting_iterator<It>` declarations below but not with these?


================
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));
----------------
Quuxplusone wrote:
> 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.
You're right, that's entirely better since it doesn't force us to reduce test coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806