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–14

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–14

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.