Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ranges::istream_view #1334

Merged
merged 9 commits into from
Oct 29, 2020
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Oct 1, 2020

Partially addresses #39

@miscco miscco requested a review from a team as a code owner October 1, 2020 21:48
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream_death/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream_death/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream_death/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Oct 2, 2020
@CaseyCarter CaseyCarter mentioned this pull request Oct 2, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 3, 2020

I am slightly confused about the Compiler error. Will investigate after the weekend when I am back at home/office

@miscco
Copy link
Contributor Author

miscco commented Oct 5, 2020

@StephanTLavavej @CaseyCarter Can you help me understand the horrors of /Za or tell me what it complains about. I am unsure what it is really complaining about

@AdamBucior
Copy link
Contributor

Try replacing basic_istream<char> with istream. Maybe it gets confused by < and >.

@CaseyCarter CaseyCarter self-assigned this Oct 7, 2020
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicky things: I'll apply these and look at constexpr tests so we can get this merged.

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Show resolved Hide resolved
tests/std/tests/P0896R4_views_istream/test.cpp Outdated Show resolved Hide resolved
* Include the number of LWG-3489
* Rename test folders from `views_istream` to `istream_view` since there is no `std::ranges::view::istream`
* Minimal constexpr test coverage
* Remove behavior extension that makes default-constructed `basic_istream_view` model range
@CaseyCarter CaseyCarter removed their assignment Oct 12, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 13, 2020

Thanks a lot for pushing this forward.

I had some minor nits ;)

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - I'll push changes to fix a few nitpicks.

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_istream_view/test.cpp Show resolved Hide resolved
using ranges::basic_istream_view;

// validate type properties
using R = basic_istream_view<T, char>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observe that this doesn't test character types other than char, much less custom traits, so there are certain kinds of product bugs that this could miss - however, the product code appears to be correct, so I don't think such test coverage is necessary to proceed. Something to keep in mind for the future, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the thought was that every wild character type would actually only implement the basic_istream functionality not the istream_view itself

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly concerned about typos where we should have said _Elem but instead said char, so it doesn't work with wchar_t.

@StephanTLavavej StephanTLavavej removed their assignment Oct 27, 2020
@StephanTLavavej StephanTLavavej self-assigned this Oct 29, 2020
@StephanTLavavej StephanTLavavej merged commit b8bae92 into microsoft:master Oct 29, 2020
@StephanTLavavej
Copy link
Member

Thanks for implementing this Halloween-themed iscream_view! 🙀 🎃

@miscco miscco deleted the istream_view branch October 29, 2020 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants