This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] implement `std::views::istream`
ClosedPublic

Authored by huixie90 on Sep 5 2022, 8:47 AM.

Details

Reviewers
philnik
var-const
ldionne
Group Reviewers
Restricted Project
Commits
rG96a509bca28b: implement `std::views::istream`
Summary

implement std::ranges::basic_istream_view and std::views::istream. Although the view itself is constexpr,
the constructor argument is a base class std::istream where its ctor/dtor are not constexpr. So no tests are performed in
constexpr

After the discussion with Louis and a survey of other implementations, only <iosfwd> is included in the header. As a result, istream_view cannot be used by just including <ranges>. Even a declaration would result in error.

#include <ranges>
void f(std::ranges::istream_view<int>);  // error. It does not pass concept check because `>>` is defined in <istream>

User will need to include <istream> to be able to use it.

Diff Detail

Event Timeline

huixie90 created this revision.Sep 5 2022, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:47 AM
Herald added a subscriber: mgorny. · View Herald Transcript
huixie90 requested review of this revision.Sep 5 2022, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I mainly looked at it out of curiosity and didn't do a full review.

libcxx/docs/Status/Cxx20Papers.csv
111

We don't add a version number for In progress items.

111

How about adding the details of the status to the ranges status page?

huixie90 updated this revision to Diff 458045.Sep 5 2022, 11:07 AM
huixie90 marked 2 inline comments as done.

address review feedback

I haven't looked very closely at the tests yet.

libcxx/docs/Status/Cxx20Papers.csv
111

If the paper is in-progress we shouldn't have a release version here. (I know it's that way in a few other places, but the numbers are completely useless)

libcxx/include/__ranges/istream_view.h
32

We want to use >= for the version checks now. I'd also move the version check outside the std namespace. It probably doesn't make much of a difference w.r.t. parsing speed, but it really doesn't hurt either.

51

_LIBCPP_HIDE_FROM_ABI

57

Probably doesn't do much, but doesn't hurt either.

105

I'm not sure the extra indirection is worth it. Why do you need to strip cv-qualifiers at all?

libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp
14

AFAIK we don't separate the tested header from the other ones.

libcxx/test/std/ranges/range.factories/range.istream.view/general.pass.cpp
21
libcxx/test/std/ranges/range.factories/range.istream.view/iterator/deref.pass.cpp
52

missing newline

libcxx/test/std/ranges/range.factories/range.istream.view/iterator/member_types.compile.pass.cpp
25

Could you also test a valid one?

libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp
16

Why don't you test Traits in any way?

huixie90 updated this revision to Diff 458255.Sep 6 2022, 1:16 PM
huixie90 marked 8 inline comments as done.

addressed feedback

libcxx/include/__ranges/istream_view.h
105

Because usually _Up is deduced to std::istringstream&, and a reference type doesn't have nested member typedef typename _Up::char_type
Note I have to strip cv for this reason in lots of places within this function

libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp
16

The standard doesn't have any constraint on Traits other than being used in std::basic_istream<CharT, Traits>. However, basic_istream is a classic template which isn't constrained. passing an invalid Traits to std::basic_istream would just hard error. so
static_assert(!HasIstreamView<int, char, NotTraits>) would just hard error

huixie90 updated this revision to Diff 458651.Sep 7 2022, 10:41 PM

added transitive includes to istream

huixie90 edited the summary of this revision. (Show Details)Sep 7 2022, 10:46 PM
huixie90 retitled this revision from implement `std::views::istream` to [libcxx][ranges] implement `std::views::istream`.Sep 11 2022, 8:47 AM
huixie90 retitled this revision from [libcxx][ranges] implement `std::views::istream` to [libc++][ranges] implement `std::views::istream`.
ldionne requested changes to this revision.Sep 30 2022, 9:31 AM

This looks pretty good!

libcxx/docs/Status/RangesIssues.csv
1 ↗(On Diff #458651)

Not for this review: I think we should get rid of this file entirely, since it's only a duplicate of information we already have in CXXYZIssues.csv?

libcxx/docs/Status/RangesPaper.csv
162 ↗(On Diff #458651)

The plan was also to get rid of this file once the One Ranges Proposal had been completed. It was basically used to track sub parts of the One Ranges Proposal.

@var-const WDYT, can we remove this now?

libcxx/include/__ranges/istream_view.h
43

Can you confirm whether you have a test that this is explicit?

51

Do you have a test that this is noexcept?

72

I think this is a great "catch", in the sense that we usually forget to use _LIBCPP_HIDE_FROM_ABI on = default functions, but we should.

101–102
#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
...
#endif

You will probably need to update the tests with that in mind, too? The test macro to use is:

#ifndef TEST_HAS_NO_WIDE_CHARACTERS
...
#endif

If you have a test that is *only* meaningful for wide characters (e.g. the test file is only checking wchar_t related stuff), then you can use // UNSUPPORTED: no-wide-characters to disable the whole test in that case. That's better than wrapping the whole test with #ifndef TEST_HAS_NO_WIDE_CHARACTERS.

This is tested in the CI, so I would assume this would have failed in the CI (although our CI is not perfect in that regard because it uses a platform that actually provides wchar_t but we make it an error to include <wchar.h>).

113

I *think* this should be constexpr, although I'm not sure this is observable.

libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp
2

General comment that applies to (probably) all tests: let's make sure we also test wchar_t and wistream_view. You should probably templatize your tests on CharT and use a mechanism similar to what @Mordante does for format.

This revision now requires changes to proceed.Sep 30 2022, 9:31 AM
var-const accepted this revision as: var-const.Sep 30 2022, 5:00 PM

I have just a few nitpicks; LGTM once the existing feedback is addressed.

libcxx/docs/Status/RangesPaper.csv
162 ↗(On Diff #458651)

Yes, I'll do it next week.

libcxx/include/__ranges/istream_view.h
57

Can you add a libc++-specific test for this optimization?

libcxx/test/std/ranges/range.factories/range.istream.view/iterator/increment.pass.cpp
49

Nit: return 0; (here and elsewhere).

libcxx/test/std/ranges/range.factories/range.istream.view/iterator/member_types.compile.pass.cpp
16

Nit: unnecessary blank line.

libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp
21

Nit: unmovable Val?

22

Ultranit: s/UnMovable/Unmovable/.

huixie90 updated this revision to Diff 464658.Oct 3 2022, 5:32 AM
huixie90 marked 13 inline comments as done.

address feedback

huixie90 updated this revision to Diff 464722.Oct 3 2022, 9:58 AM
huixie90 marked an inline comment as done.

next try

I didn't do a review, but mainly had a look why the CI fails.

libcxx/include/module.modulemap.in
1017

See the comment in ranges why this should be disabled when localization is disabled.

libcxx/include/ranges
304

Maybe move this to the end. This header "works", but can't be used when localization isn't enabled. The header includes <iosfwd> which is fine when no locales are used. However when using the "contents" of the istream_view.h header you need to include <istream>, which indirectly includes <ios> and results in an error in that header on line 216.

So I think it would be best to not provide this header when localization is disable.

I use a similar approach in <chrono> where I don't include headers when localization is disabled. (There are more conditions there, but localization is a reason on itself.)

libcxx/test/libcxx/ranges/range.factories/range.istream.view/no_unique_address.compile.pass.cpp
10

The same for the other tests.

libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp
26

This macro is never defined since "test_macros.h" isn't included.

huixie90 updated this revision to Diff 464761.Oct 3 2022, 12:11 PM
huixie90 marked 3 inline comments as done.

Fix CI

huixie90 updated this revision to Diff 464893.Oct 3 2022, 11:52 PM
huixie90 marked an inline comment as done.

next try

huixie90 edited the summary of this revision. (Show Details)Oct 4 2022, 1:59 AM
huixie90 edited the summary of this revision. (Show Details)
ldionne accepted this revision.Oct 6 2022, 9:13 AM
ldionne added inline comments.
libcxx/docs/Status/RangesPaper.csv
162 ↗(On Diff #464893)

Newline!

libcxx/test/std/ranges/range.factories/range.istream.view/utils.h
9

For functions we generally use snake_case, although we might not be 100% consistent everywhere. I think it would still be better to try to stick to snake_case.

This revision is now accepted and ready to land.Oct 6 2022, 9:13 AM
huixie90 updated this revision to Diff 465841.Oct 6 2022, 1:00 PM
huixie90 marked 2 inline comments as done.
huixie90 edited the summary of this revision. (Show Details)

rebase

This revision was automatically updated to reflect the committed changes.