This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove incorrect default constructor in cpp17_input_iterator

Authored by ldionne on Dec 15 2021, 8:33 AM.



AFAICT, Cpp17InputIterators are not required to be default constructible,
so our archetype should not be default constructible. Removing that
constructor has a ripple effect on a couple of tests that were making
incorrect assumptions. Notably:

  • Some tests were using cpp17_input_iterator to form a common_range using test_common_range. That is not valid, because a cpp17_input_iterator is not a sentinel for itself after the change (it's not semiregular anymore).
  • Some tests were using cpp17_input_iterator as a sentinel for itself (basically the same problem as above), which is invalid for the same reason.
  • This change required modifying sentinel_wrapper so that it can work with non-default-constructible iterators like cpp17_input_iterator. Otherwise, there would be no way to test most utilities on those iterators.

Diff Detail

Event Timeline

ldionne requested review of this revision.Dec 15 2021, 8:33 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 8:33 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 396539.Dec 29 2021, 8:10 AM

Rebase onto main and fix additional tests. Everything should pass now.

Quuxplusone requested changes to this revision.Dec 29 2021, 9:13 AM
Quuxplusone added a subscriber: Mordante.

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.


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?


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


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

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.


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


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 .

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


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?


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.


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


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<NonCommon<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_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<NonCommon<forward_iterator<int*>, sentinel_wrapper<forward_iterator<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)

This revision now requires changes to proceed.Dec 29 2021, 9:13 AM
ldionne marked 6 inline comments as done.Jan 3 2022, 6:39 AM

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.


Yeah, I think that makes sense.


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.


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?


You're right, that's entirely better since it doesn't force us to reduce test coverage.

ldionne updated this revision to Diff 397058.Jan 3 2022, 6:40 AM
ldionne marked 3 inline comments as done.

Apply most review comments.

My comment in 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.


Ping me when this lands and I'll volunteer to do it. :)


"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, 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. :)

ldionne updated this revision to Diff 397103.Jan 3 2022, 10:34 AM
ldionne marked 5 inline comments as done.

Address remaining comments. I thought I had done everything for some reason, but didn't.

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


I've returned to auto x = Foo(...) because it's what's done elsewhere in this test.


Sorry, I forgot to address this comment. It should be addressed in the latest update.


This one can actually be removed entirely.


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

Quuxplusone added inline comments.Jan 3 2022, 3:15 PM

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


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]);


check_sentinel<true>(forward_iterator<int*>(range), 3);

"There's no such thing as an archetype" alert! :) This comment is describing lack of test coverage, so it would be better worded as

// we no longer have a test iterator that is both input and default-constructible, so not testing that case

You could use something like test<std::istream_iterator<int>>(); here, to avoid losing test coverage, but that might require dragging in <istream> to get it to compile. Really I'm just carping about the excuse-making wording of the proposed comment. It's not like it's C++'s fault that we lost this coverage.

ldionne marked 2 inline comments as done.Jan 4 2022, 8:13 AM
ldionne added inline comments.

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.


Let's keep this refactoring for a subsequent patch, the same where I can remove range_t.

ldionne updated this revision to Diff 397307.Jan 4 2022, 8:13 AM
ldionne marked an inline comment as done.

Address review comments

Quuxplusone accepted this revision.Jan 4 2022, 8:19 AM
Quuxplusone added inline comments.

Argh, gross. I guess there's really no good way to make a type that says "I am assignable to this unrelated type." Making it implicitly-convertible is the best we can do. OK.


Optional: leave a TODO comment here that we ought to get rid of these bogus operator== in a later patch.

This revision is now accepted and ready to land.Jan 4 2022, 8:19 AM
ldionne added inline comments.Jan 4 2022, 9:32 AM

I already have another patch in the works to rewrite that whole test.

The CI failure seems like a flake due to the new (and slow) nodes we're using right now. Shipping this.

This revision was landed with ongoing or failed builds.Jan 4 2022, 11:33 AM
This revision was automatically updated to reflect the committed changes.