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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

My comment in `range.iter.ops.next/iterator_sentinel.pass.cpp` is still unaddressed, but I'm also fine with a TODO comment there. Again, my goal there is that we should never instantiate any `foo_iterator<T>` where `T` is a non-iterator (sentinel) type.



================
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};
+
----------------
ldionne wrote:
> 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.
Ping me when this lands and I'll volunteer to do it. :)


================
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);
----------------
ldionne wrote:
> 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?
"What criteria do you use" — Largely random. Probably affected by "how hard is it for my eyes to pick out the name of the variable among all the other tokens" (so arguably `It first(` is more lost-in-the-woods than `It> first(` on line 62). Probably affected by "how much is it obvious whether the explicit-cast behavior is intentional or not," e.g. `It first(range.begin())` could plausibly be a "typo" for `It first = range.begin()`, but `stride_counting_iterator<It> first(It(range.begin()))` is clearly not intended to be the same as `stride_counting_iterator<It> first = It(range.begin())`. Mostly affected by the phase of the moon. :)
In an ideal world I'd absolutely [use = for all initializations](https://quuxplusone.github.io/blog/2019/02/18/knightmare-of-initialization/#simple-guidelines-for-variable-i), but that weighs against consistency with existing test code (where a lot of tests use parens-initialization), so when I comment inconsistently, you're not seeing inconsistency of //my preference// so much as inconsistency of my //inclination to leave a nitty comment//. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806