This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Clean up test_iterators.h. NFCI.
ClosedPublic

Authored by Quuxplusone on May 28 2021, 1:58 PM.

Details

Reviewers
cjdb
ldionne
zoecarver
Group Reviewers
Restricted Project
Commits
rG7ba3627b5464: [libc++] Clean up test_iterators.h. NFCI.
Summary
The majority of the changes here are whitespace.
Also simplify `ThrowingIterator`'s bookkeeping (NFC).
Also move some free operators into hidden friends, for sanity's sake.
Also `=delete` some more comma operators.
Also use `constexpr` in C++20 instead of `TEST_CONSTEXPR_CXX14`.

Diff Detail

Event Timeline

zoecarver requested review of this revision.May 28 2021, 1:58 PM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2021, 1:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.May 28 2021, 1:59 PM
libcxx/test/support/test_iterators.h
693

I need to find and remove the matching comment.

721

We need some similar changes here.

cjdb added a comment.May 28 2021, 2:03 PM

The suggested changes from D102020 don't properly consider the design of this type, nor its surrounding ecosystem. Please do not merge it, and instead use the appropriate helper types that are already in test_iterators.h.

libcxx/test/support/test_iterators.h
634–637

Why? This is a move-only iterator, which only makes sense in ranges land.

638–641

Why?

665

This overload needs to keep its [[nodiscard]].

666–668

Iterators don't have sentinel sub-types. We have a sentinel_wrapper for that.

cjdb added inline comments.May 28 2021, 2:36 PM
libcxx/test/support/test_iterators.h
638–641

Upon further thought, removing this opens a gaping hole for when a I satisfies std::input_iterator, but not _`cpp17-input-iterator`_ and doesn't result in the primary template for iterator_traits.

libcxx/test/support/test_iterators.h
634–637

It makes sense anywhere it compiles, which is to say >=14.

638–641

'14-compatibility.

665

IMO, no, the [[nodiscard]] was just noise. We don't need to [[nodiscard]] everything in sight: we don't [[nodiscard]] operator*, for example, even though "obviously" any test code that actually discarded the result of *it would probably have a bug, just as much as any test code that actually discarded the result of it.base().

666–668

Hm. I don't particularly dislike sentinel_wrapper. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives

some_function<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
some_function<cpp20_input_iterator<int*>, cpp20_input_iterator<int*>::sentinel>();

(Right now sentinel_wrapper is used only in "range.iter.ops.advance/advance.pass.cpp".)

I'm okay eliminating this diff for now, pending some examples of how sentinel{,_wrapper} would be used in real test cases. Certainly if we're adding sentinel we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.

858–859

Please void operator&() const DELETE_FUNCTION; while you're at it.

cjdb added inline comments.May 28 2021, 3:08 PM
libcxx/test/support/test_iterators.h
634–637

Yeah, no. This isn't useful in C++14, so it shouldn't be available. Not to mention that it depends on std::iter_value_t and std::iter_difference_t.

666–668

Hm. I don't particularly dislike sentinel_wrapper. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives

sentinel_wrapper is the sentinel version of our iterator wrappers; if your hard disk has a limited amount of free space where each byte counts, you'd be doing yourself a disservice as we'd need to duplicate this code for each of our other iterators too.

(Right now sentinel_wrapper is used only in "range.iter.ops.advance/advance.pass.cpp".)

It's also used in range.iter.ops/range.iter.ops.next. I should fix up range.iter.ops/range.iter.ops.prev to use it too.

I'm okay eliminating this diff for now, pending some examples of how sentinel{,_wrapper} would be used in real test cases.

If, by "real test cases", you mean "more test cases" (those places where it's used today seem real to me): those should start to flow in when algorithms start to get committed.

Certainly if we're adding sentinel we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.

It's a bit hard to refactor code that doesn't exist yet, but here's an example of what might exist in the near future.

auto v = std::vector{1, 2, 3};
std::ranges::find(v.begin(), sentinel_wrapper(v.end()), 0);
Quuxplusone commandeered this revision.Jun 1 2021, 8:07 PM
Quuxplusone planned changes to this revision.
Quuxplusone edited reviewers, added: zoecarver; removed: Quuxplusone.

Gentle ping -- are we doing this?

Quuxplusone retitled this revision from [libcxx][nfc] Cleanup cpp20_input_iterator. to [libc++] Clean up test_iterators.h. NFCI..
Quuxplusone edited the summary of this revision. (Show Details)

Are we doing this?

Basically nah; some of this has already been committed elsewhere (e.g. removing [[nodiscard]]) and the rest is not obviously good anymore.
But I've just done a new pass over test_iterators.h and recommend these new changes. There was a surprising amount of misindentation in ThrowingIterator!

ldionne accepted this revision.Sep 7 2021, 10:42 AM

LGTM when CI passes.

I'd rather keep the old formulations for operations in ThrowingIterator, but it's non-blocking.

libcxx/test/support/test_iterators.h
563

I'd rather not make this change here and instead keep the previous formulations for if (action_ == ...) if (index_ == 0) ...

This revision is now accepted and ready to land.Sep 7 2021, 10:42 AM

Oops, contiguous_iterator's operator, was already deleted (just not at the bottom of the class like the others).
Considered moving all the other iterators' operators into hidden friends (above their deleted operator,s), and then decided that that was too big an edit; might make that PR later. I don't immediately see why random_access_iterator<T> should be comparable to random_access_iterator<U>, but I could imagine us relying on that somehow in some test.

This time for sure!
(Rename a nested template parameter T to T2.)

Rebase on main after landing D109414. Buildkite should be happy now.

n+1th time's the charm!

This revision was automatically updated to reflect the committed changes.