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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Geez, I've been having issues with Phabricator. Submitting comments.



================
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:
> 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//. :)
I've returned to `auto x = Foo(...)` because it's what's done elsewhere in this test.


================
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);
   }
 
----------------
Quuxplusone wrote:
> 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.)
Sorry, I forgot to address this comment. It should be addressed in the latest update.


================
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));
----------------
Quuxplusone wrote:
> 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`?
This one can actually be removed entirely.


================
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>);
----------------
Quuxplusone wrote:
> 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)
I actually went a bit further and refactored this whole thing some more. I'll update it, let me know if you want it in a separate review.

I don't think we're losing important coverage here by removing the check for `common_range<Common<cpp17_input_iterator>>`, since if someone wants to make a `cpp17_input_iterator` that's also a sentinel for itself, they have basically created a `forward_iterator` (that's not 100% accurate, but it's pretty close).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115806