This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement Ranges changes to `istream{,buf}_iterator`.
ClosedPublic

Authored by var-const on Feb 11 2022, 11:07 PM.

Details

Summary

The changes from the One Ranges Proposal amount to adding:

  • a constructor that takes a default_sentinel_t and is equivalent to the default constructor;
  • an operator== that compares the iterator to default_sentinel_t.

The original proposal defined two overloads for operator== (different
argument order) as well as operator!=. This has been removed by
P1614.

Diff Detail

Event Timeline

var-const requested review of this revision.Feb 11 2022, 11:07 PM
var-const created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 11:07 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Use the actual patch number.

Mark changes to stream iterators as done.

var-const added inline comments.Feb 11 2022, 11:15 PM
libcxx/include/__iterator/istream_iterator.h
50

Note: the default constructor and this constructor are defined in the same clause in the Standard (23.6.2.2.1, http://eel.is/c++draft/istream.iterator#cons-1), so it made sense to define one in terms of the other.

libcxx/include/__iterator/istreambuf_iterator.h
97

This is the definition from the Standard verbatim (http://eel.is/c++draft/istreambuf.iterator#ops-7). At least conceptually, it involves creating a istreambuf_iterator from the sentinel. We could instead do something like return __test_for_eof();. I'm not sure if it's worth it, though.

libcxx/test/std/iterators/stream.iterators/istream.iterator/istream.iterator.cons/default.pass.cpp
57

Because these constructors (default constructor and default_sentinel_t constructor) are defined in the same clause, I'm leaning towards testing them in the same test file.

Add noexcept tests.

Small fixes to the synopsis.

• Quuxplusone accepted this revision.Feb 13 2022, 11:15 AM
• Quuxplusone added a subscriber: • Quuxplusone.

LGTM % minor comments!
Please fix (especially the operator!=s), rebase and re-upload to see a green CI run (now that the ASAN failures should be fixed, right?), but then this seems ready to ship.

libcxx/include/__iterator/istream_iterator.h
77

Nit: I know the standard says !in_stream, but can we please make this return __i.__in_stream_ == nullptr; for clarity? (Since it's just a raw pointer, there's no difference except readability.)

91–98

operator!= should be under #if _LIBCPP_STD_VER <= 17, because it's gone in '20.

libcxx/include/__iterator/istreambuf_iterator.h
70
97

I agree; but I think it is worth it, just to get rid of a layer of indirection and avoid having to name the __s parameter. :) return __i.__test_for_eof(); sounds like a win to me.

108–112

As above, this operator!= is gone in '20.

libcxx/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/default.pass.cpp
13–15

Nit: To match the synopsis comment, and the throw()/noexcept distinction isn't terribly important IMHO.

This revision is now accepted and ready to land.Feb 13 2022, 11:15 AM
• Quuxplusone requested changes to this revision.Feb 13 2022, 11:19 AM

Er, actually, I spoke too soon...! There are also test failures on Apple, because you changed the operator==/!= tests to include references to default_sentinel, and default_sentinel is disabled when libcpp-has-no-concepts.
I suggest fixing this by changing <__iterator/default_sentinel.h> to enable default_sentinel in C++20 regardless of _LIBCPP_HAS_NO_CONCEPTS — it should merely be guarded by #if _LIBCPP_STD_VER > 17 and nothing else. However, now this has become a more involved change, and I think at least I should re-review it once the CI is green, and perhaps also someone else.

(Alternatively, you could split out the default_sentinel-related tests into separate test files and skip them for libcpp-has-no-concepts targets. But that seems yuckier.)

This revision now requires changes to proceed.Feb 13 2022, 11:19 AM
var-const marked 6 inline comments as done.Feb 14 2022, 12:19 PM

I suggest fixing this by changing <__iterator/default_sentinel.h> to enable default_sentinel in C++20 regardless of _LIBCPP_HAS_NO_CONCEPTS — it should merely be guarded by #if _LIBCPP_STD_VER > 17 and nothing else.

Done. Since it doesn't actually use concepts, that seems reasonable.

libcxx/include/__iterator/istream_iterator.h
77

Sure.

91–98

Done (I don't think this is related to the One Ranges proposal, but it makes sense to do these changes in one go). Also mentioned this in the synopsis.

libcxx/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/default.pass.cpp
13–15

Done. I've done the same change in the synopsis -- you intended to change both, right?

var-const marked 3 inline comments as done.Feb 14 2022, 12:19 PM

Address feedback (in particular, remove the throw()/noexcept distinction
from the synopsis and guard default_sentinel on the language version, not the
availability of concepts).

ldionne accepted this revision as: ldionne.Feb 14 2022, 1:28 PM
ldionne added a subscriber: ldionne.

This LGTM. I'm fine with not guarding default_sentinel_t under _LIBCPP_HAS_NO_CONCEPTS, since (1) that macro will soon die and (2) it technically doesn't need concepts.

Leaving final approval to Arthur since he had comments.

libcxx/test/std/iterators/stream.iterators/istream.iterator/istream.iterator.cons/default.pass.cpp
57

I have a very slight preference for different files since they are really different constructors (except they happen to have the same effect). But definitely not a blocking comment.

libcxx/test/std/iterators/stream.iterators/istream.iterator/istream.iterator.ops/equal.pass.cpp
11

If you add the test here (I'd weakly prefer another file), please update the synopsis. This applies in a couple other tests too.

Address feedback, fix the CI.

Rebase on main.

• Quuxplusone accepted this revision.Feb 15 2022, 7:56 AM

I just landed D118736, so please check relatively carefully for merge conflicts; but I don't think you'll find any.

This revision is now accepted and ready to land.Feb 15 2022, 7:56 AM
var-const updated this revision to Diff 408913.Feb 15 2022, 9:15 AM

Rebase on main.