This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][nfc] prefixes test type `input_iterator` with `cpp17_`
ClosedPublic

Authored by cjdb on Apr 24 2021, 2:40 PM.

Details

Summary

C++20 revised the definition of what it means to be an iterator. While
all _Cpp17InputIterators_ satisfy std::input_iterator, the reverse
isn't true. D100271 introduces a new test adaptor to accommodate this
new definition (cpp20_input_iterator).

In order to help readers immediately distinguish which input iterator
adaptor is _Cpp17InputIterator_, the current input_iterator adaptor
has been prefixed with cpp17_.

Diff Detail

Event Timeline

cjdb created this revision.Apr 24 2021, 2:40 PM
cjdb requested review of this revision.Apr 24 2021, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2021, 2:40 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone resigned from this revision.Apr 24 2021, 3:12 PM
Quuxplusone added a subscriber: Quuxplusone.

It seems like literally all this does is make the test iterators' names longer and more tedious, so I personally don't think we should accept this patch.

cjdb updated this revision to Diff 340317.Apr 24 2021, 5:10 PM
cjdb edited the summary of this revision. (Show Details)

benchmark file was not updated (tooling only considered files in libcxx/test/)

I had a short look at the patch. I also wonder whether this is the direction we want to go.
For reviewing, I assume this is all basically a search and replace? Or are there more changes?

libcxx/test/support/test_iterators.h
1

The new C++20 style iterators will they also be put in this file or will they get their own header. If the latter then maybe rename this file to test_legacy_iterator.h.

cjdb added a comment.Apr 26 2021, 9:59 AM

I had a short look at the patch. I also wonder whether this is the direction we want to go.

The intention of this patch is to optimise for the reader, not the writer. I don't want any contributors or reviewers to question whether input_iterator means legacy input iterator or C++20 input iterator.

For reviewing, I assume this is all basically a search and replace? Or are there more changes?

Should be a 100% search and replace with one file renamed too (input_iterator.h -> legacy_input_iterator.h).

libcxx/test/support/test_iterators.h
1

My intention was to add them here and guard them with a ranges support macro. C++20 ranges may want to test both in certain circumstances.

ldionne requested changes to this revision.Apr 26 2021, 10:03 AM

I agree this is making names longer and I dislike that. However, I think being able to distinguish between an archetype for a C++20 input_iterator and a pre-concepts InputIterator archetype is useful and necessary in the test suite, since those two concepts are different (see note about equality comparable https://en.cppreference.com/w/cpp/iterator/input_iterator).

One other option would be to call those cpp17_XXXXX_iterator instead. That way, if we introduce new iterator concepts eventually in the future, we can call the current ones cpp20_XXXXX_iterator instead of being stuck with a naming problem. WDYT?

I also agree with @Mordante , if we're going to rename the header in the future, we might just as well rename it now. If there's no plans to rename the header, then this LGTM.

Requesting changes for discussion.

This revision now requires changes to proceed.Apr 26 2021, 10:03 AM

The intention of this patch is to optimise for the reader, not the writer.

As a reader myself, I explicitly state that this patch does not optimize for my preferences when reading code. It makes the code actively harder to read.

cjdb added a subscriber: tcanens.Apr 26 2021, 10:10 AM

I agree this is making names longer and I dislike that. However, I think being able to distinguish between an archetype for a C++20 input_iterator and a pre-concepts InputIterator archetype is useful and necessary in the test suite, since those two concepts are different (see note about equality comparable https://en.cppreference.com/w/cpp/iterator/input_iterator).

One other option would be to call those cpp17_XXXXX_iterator instead. That way, if we introduce new iterator concepts eventually in the future, we can call the current ones cpp20_XXXXX_iterator instead of being stuck with a naming problem. WDYT?

I have grave objections to cpp17 as a prefix (I even raised that in LWG once). It's weird see a C++11 test to have a cpp17_XXXX_iterator. I'm pretty sure that's why @tcanens pushed so strongly for LegacyXXXXIterator on cppreference too.

I also agree with @Mordante , if we're going to rename the header in the future, we might just as well rename it now. If there's no plans to rename the header, then this LGTM.

No plans to rename the header, but I could be talked into it.

I agree this is making names longer and I dislike that. However, I think being able to distinguish between an archetype for a C++20 input_iterator and a pre-concepts InputIterator archetype is useful and necessary in the test suite, since those two concepts are different (see note about equality comparable https://en.cppreference.com/w/cpp/iterator/input_iterator).

@cjdb Are the other iterator concepts also different between C++20 and C++17? It's true for input iterators, but is it also the case for e.g. forward iterator? I think that should influence what we do here.

cjdb added a comment.Apr 26 2021, 10:23 AM

I agree this is making names longer and I dislike that. However, I think being able to distinguish between an archetype for a C++20 input_iterator and a pre-concepts InputIterator archetype is useful and necessary in the test suite, since those two concepts are different (see note about equality comparable https://en.cppreference.com/w/cpp/iterator/input_iterator).

@cjdb Are the other iterator concepts also different between C++20 and C++17? It's true for input iterators, but is it also the case for e.g. forward iterator? I think that should influence what we do here.

Contrast:

tl;dr yes. At minimum, the iterator tags are now mandatory.

One other option would be to call those cpp17_XXXXX_iterator instead. That way, if we introduce new iterator concepts eventually in the future, we can call the current ones cpp20_XXXXX_iterator instead of being stuck with a naming problem. WDYT?

I have grave objections to cpp17 as a prefix (I even raised that in LWG once). It's weird see a C++11 test to have a cpp17_XXXX_iterator. I'm pretty sure that's why @tcanens pushed so strongly for LegacyXXXXIterator on cppreference too.

I'm not too fond of the name Legacy. Unless I'm mistaken the standard uses Cpp17 for these iterators. So that would be my preference.

I also agree with @Mordante , if we're going to rename the header in the future, we might just as well rename it now. If there's no plans to rename the header, then this LGTM.

No plans to rename the header, but I could be talked into it.

I have a strong opinion about the renaming of the header. Just wanted to make sure there wouldn't be a second update in the near future.

Quuxplusone requested changes to this revision.Apr 26 2021, 11:36 AM
Quuxplusone added inline comments.
libcxx/test/support/test_iterators.h
64

@cjdb wrote:

tl;dr yes. At minimum, the iterator tags are now mandatory.

The existing iterators already have tags, though. So that's not a concern for us.

I suggested to Louis in chat but will put here for the record: If the main concern of this patch is to make sure we have a test iterator which is a "non-comparable input iterator" (comparable only to its sentinel type, but not to itself), then I suggest you leave everything the way it is in main but then add the new C++20-only, sentinel-comparison-only test iterator, like this:

#if TEST_STD_VER >= 20
template<class It>
class incomparable_input_iterator {
    // This is an input iterator according to C++>=20, but not according to C++<=17.
    // It is comparable with its sentinel type, but not with its own type.
    [...mostly the same stuff as the regular input iterator test type...]
public:
    struct sentinel {
        constexpr explicit sentinel(It it) : it_(it) {}
    };
    friend constexpr bool operator==(const incomparable_input_iterator& x, const sentinel& y)
        {return x.it_ == y.it_;}
};
#endif

This probably belongs right above contiguous_iterator, circa line 315, because we intend it to be new in C++20.

cjdb added inline comments.Apr 26 2021, 12:06 PM
libcxx/test/support/test_iterators.h
64

If the main concern of this patch is to make sure we have a test iterator which is a "non-comparable input iterator" (comparable only to its sentinel type, but not to itself)

This isn't the only difference between Cpp17InputIterator and std::input_iterator.

I suggest you leave everything the way it is in main but then add the new C++20-only, sentinel-comparison-only test iterator

That leaves ambiguity as to whether or not input_iterator refers to a Cpp17InputIterator or is a strict std::input_iterator. It needs to be renamed to clear up that ambiguity.

libcxx/test/support/test_iterators.h
64

This isn't the only difference

Obligatory "What are the differences you care about, then?"
No two things in the world are ever exactly the same. That doesn't mean we make libc++ PRs about it. :)

cjdb added inline comments.Apr 26 2021, 1:28 PM
libcxx/test/support/test_iterators.h
64

The most salient difference is that C++20 input iterators need not be copyable. I expected that this would be fresh in your mind, since you've recently reviewed D100271, which is why I didn't mention it.

cjdb updated this revision to Diff 340647.Apr 26 2021, 2:20 PM
cjdb retitled this revision from [libcxx][nfc] renames test iterator types to `legacy_*_iterator` to [libcxx][nfc] prefixes test type `input_iterator` with `cpp17_`.
cjdb edited the summary of this revision. (Show Details)
  • reverts legacy_forward_iterator (and stronger)
  • replaces legacy prefix with cpp17
  • revises commit message to reflect drastic changes
libcxx/test/support/test_iterators.h
64

So basically the intent of D101242-plus-D100271 is to introduce moveonly_input_iterator (and moveonly_output_iterator) while leaving everything else the same? I would approve of that direction.

But I notice that D100271 doesn't introduce a sentinel with which its new iterator type could be compared. It probably should include a sentinel, in order to make the new iterator type more useful with C++20 algorithms like std::ranges::copy. Still, I'm a little leery of introducing too many knobs. We probably don't want all four of a "copyable, comparable iterator" (i.e. the existing input_iterator/output_iterator) and a "move-only, comparable iterator" and a "copyable, sentineled iterator" and a "move-only, sentineled iterator"; but I'm still not sure what we do want.

(Fortunately, std::forward_iterator and higher all require std::regular, which means "copyable and comparable"; so the above combinatorial explosion applies only to input iterators and output iterators.)

cjdb added inline comments.Apr 26 2021, 4:20 PM
libcxx/test/support/test_iterators.h
64

So basically the intent of D101242-plus-D100271 is to introduce moveonly_input_iterator (and moveonly_output_iterator) while leaving everything else the same? I would approve of that direction.

That + prefixing current input_iterator with cpp17_ (or copyable_, which is more descriptive, but longer).

But I notice that D100271 doesn't introduce a sentinel with which its new iterator type could be compared. It probably should include a sentinel, in order to make the new iterator type more useful with C++20 algorithms like std::ranges::copy.

The patch has no need for a sentinel, so it's not added. I think it'd be a good idea for us to think about what the new sentinel type should do, and add it either with the first algorithm or as a follow-up patch to D100271 (it certainly shouldn't be blocking).

Still, I'm a little leery of introducing too many knobs. We probably don't want all four of a "copyable, comparable iterator" (i.e. the existing input_iterator/output_iterator) and a "move-only, comparable iterator" and a "copyable, sentineled iterator" and a "move-only, sentineled iterator"; but I'm still not sure what we do want.

Yeah, that's a bad idea. My intention is for the new, move-only input iterator is for it to be a completely minimal interface (i.e. move-only, not a self-sentinel).

ldionne accepted this revision.Apr 27 2021, 12:21 PM

Reached agreement with @cjdb offline about using cpp17_input_iterator here and cpp20_input_iterator for the new thing introduced by D100271.

LGTM but please add a one-line comment saying This is the InputIterator concept as described prior to C++20 or something along those lines.

Mordante accepted this revision.Apr 28 2021, 10:18 AM

Reached agreement with @cjdb offline about using cpp17_input_iterator here and cpp20_input_iterator for the new thing introduced by D100271.

Nice!
LGTM.

Can we apply the feedback and land this? Sorry to be pushy, but this is a large change (in terms of touched lines) and I'd like to get it in ASAP.

cjdb updated this revision to Diff 342043.Apr 30 2021, 2:31 PM

adds a comment explaining what cpp17_input_iterator represents

cjdb updated this revision to Diff 342101.Apr 30 2021, 6:46 PM

rebases to fix CI

cjdb updated this revision to Diff 342112.Apr 30 2021, 8:49 PM

tries to get CI working once more

libcxx/benchmarks/filesystem.bench.cpp
69 ↗(On Diff #342112)

This legacy_ should also be cpp17_, right? (I think a git grep would help here.)

cjdb updated this revision to Diff 342115.Apr 30 2021, 9:38 PM

fixes benchmark

libcxx/benchmarks/filesystem.bench.cpp
69 ↗(On Diff #342112)

Thanks! TIL about git grep.

This revision was not accepted when it landed; it landed in state Needs Review.May 1 2021, 10:03 PM
This revision was automatically updated to reflect the committed changes.