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.

Sign up for GitHub

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

Jump to bottom

Implement LWG-3571 and LWG-3570: flush_emit set badbit if the emit call fails #2418

Merged
merged 18 commits into from
Mar 17, 2022
Merged

Implement LWG-3571 and LWG-3570: flush_emit set badbit if the emit call fails #2418

merged 18 commits into from
Mar 17, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Dec 14, 2021 •
edited
Loading

Fixes #2396

Fixes #2395

@fsb4000 fsb4000 requested a review from a team as a code owner December 14, 2021 14:26
@fsb4000 @miscco
better name for output stream state
a6596d5 
Co-authored-by: Michael Schellenberger Costa <[email protected]>
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Dec 15, 2021
StephanTLavavej
StephanTLavavej requested changes Dec 15, 2021
View reviewed changes
stl/inc/ostream Outdated Show resolved Hide resolved
@fsb4000 fsb4000 changed the title Implement LWG-3571: flush_emit set badbit if the emit call fails Implement LWG-3571 and LWG-3570: flush_emit set badbit if the emit call fails Dec 15, 2021
@fsb4000

This comment has been minimized.

Sign in to view
MattStephanson
MattStephanson suggested changes Dec 16, 2021
View reviewed changes
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/ostream Outdated Show resolved Hide resolved
fsb4000 and others added 3 commits December 16, 2021 09:39
@fsb4000 @MattStephanson
correct sentries usage
4ecbb31 
Co-authored-by: Matt Stephanson <[email protected]>
@fsb4000 @MattStephanson
add tests
c08e789 
Co-authored-by: Matt Stephanson <[email protected]>
@fsb4000
add more tests
797fc67
MattStephanson
MattStephanson approved these changes Dec 23, 2021
View reviewed changes
Copy link
Contributor

@MattStephanson MattStephanson left a comment

Choose a reason for hiding this comment

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

LGTM!

@StephanTLavavej StephanTLavavej self-assigned this Jan 19, 2022
@StephanTLavavej
Copy link
Member

Thanks, this looks solid! The only things I noticed were stylistic nitpicks, so I went ahead and pushed a merge with main followed by those trivial changes, after validating that the relevant tests still pass.

@StephanTLavavej StephanTLavavej removed their assignment Feb 23, 2022
barcharcraz
barcharcraz suggested changes Mar 10, 2022
View reviewed changes
stl/inc/syncstream
if (!_Ok) {
_State |= ios_base::badbit;
} else {
_TRY_IO_BEGIN
Copy link
Member

Choose a reason for hiding this comment

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

God I really don't like these, it's pre-existing (and changing it would be ... a project) so don't consider this a change request, but maybe we should open an issue.

tests/std/tests/P0753R2_manipulators_for_cpp_synchronized_buffered_ostream/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Mar 11, 2022
@StephanTLavavej StephanTLavavej removed their assignment Mar 11, 2022
@StephanTLavavej
Copy link
Member

Thanks - I approve of the ternary-to-if change that @barcharcraz requested, as if this test ever fails, the asserting line will indicate what was expected - same as the rationale for preferring assert(is_cute()); assert(is_fluffy()); assert(is_kitten()); on separate lines instead of assert(is_cute() && is_fluffy() && is_kitten());.

@barcharcraz barcharcraz removed their assignment Mar 12, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej self-assigned this Mar 15, 2022
@StephanTLavavej StephanTLavavej merged commit e6fa258 into microsoft:main Mar 17, 2022
@StephanTLavavej
Copy link
Member

Thanks for syncing the implementation with the Standard's stream of LWG issues! 😹 🚀 🎉

@fsb4000 fsb4000 deleted the fix2396 branch March 17, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miscco miscco miscco left review comments

@MattStephanson MattStephanson MattStephanson approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@barcharcraz barcharcraz barcharcraz approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
LWG Library Working Group issue
Projects
None yet
Milestone
No milestone
5 participants
@fsb4000 @StephanTLavavej @barcharcraz @miscco @MattStephanson