Skip to content

Implement P2432R1 Fix istream_view#2245

Merged
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:views_istream
Oct 20, 2021
Merged

Implement P2432R1 Fix istream_view#2245
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:views_istream

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

Drive-by: Drop extraneous if constexpr condition in join_view::end as suggested by cplusplus/draft#4957.

Fixes #2240

Drive-by: Drop extraneous `if constexpr` condition in `join_view::end` as suggested by cplusplus/draft#4957.

Fixes microsoft#2240
@CaseyCarter CaseyCarter added ranges C++20/23 ranges defect report Applied retroactively labels Oct 5, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 5, 2021 02:13
Comment thread stl/inc/ranges
Copy link
Copy Markdown
Contributor

@barcharcraz barcharcraz 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

@StephanTLavavej StephanTLavavej self-assigned this Oct 13, 2021
Comment on lines +77 to +89
{ // using ranges::basic_istream_view with wide stream
wistringstream wintstream{L"0 1 2 3"};
T input_value[] = {-1, -1, -1, -1, -1};
ranges::copy(basic_istream_view<T, wchar_t>{wintstream}, input_value);
assert(ranges::equal(input_value, expected));
}

{ // using ranges::basic_istream_view with narrow stream
istringstream intstream{"0 1 2 3"};
T input_value[] = {-1, -1, -1, -1, -1};
ranges::copy(basic_istream_view<T, char>{intstream}, input_value);
assert(ranges::equal(input_value, expected));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ultra nitpicks, no change requested: (1) I observe that these are wide-then-narrow while the following tests are ordered narrow-then-wide, and (2) I observe that these tests don't verify static_assert(noexcept) while the following tests do. (For the latter, there is no loss of coverage since istream_view and wistream_view are alias templates for basic_istream_view.)

This isn't significant enough to reset testing, just wanted to comment on something. 😹

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ultra nitpicks, no change requested: (1) I observe that these are wide-then-narrow while the following tests are ordered narrow-then-wide, and (2) I observe that these tests don't verify static_assert(noexcept) while the following tests do. (For the latter, there is no loss of coverage since istream_view and wistream_view are alias templates for basic_istream_view.)

This isn't significant enough to reset testing, just wanted to comment on something. 😹

I agree this just doesn't quite rise to the level of resetting testing. I'll keep the comment open just in case someone requests another change in which case I'll address this as well.

@StephanTLavavej StephanTLavavej removed their assignment Oct 14, 2021
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Oct 14, 2021
@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2021
@StephanTLavavej
Copy link
Copy Markdown
Member

I'm mirroring this to an MSVC-internal PR; please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 4b97eaa into microsoft:main Oct 20, 2021
@StephanTLavavej
Copy link
Copy Markdown
Member

Thanks for traveling back in time to save the future! 🤖 ⏳ ✔️

@CaseyCarter CaseyCarter deleted the views_istream branch October 20, 2021 02:00
mike-goutokuji pushed a commit to mike-goutokuji/STL that referenced this pull request Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature defect report Applied retroactively ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P2432R1 Fix istream_view

4 participants