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

Properly catch exceptions in {i|o}stream #2033

Merged
merged 16 commits into from
Sep 2, 2021
Merged

Properly catch exceptions in {i|o}stream #2033

merged 16 commits into from
Sep 2, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 1, 2021

Fixes #1858

@miscco miscco requested a review from a team as a code owner July 1, 2021 19:52
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jul 2, 2021
@miscco @fsb4000
Update tests/std/tests/GH_001858_iostream_exception/test.cpp
79f724a 
Co-authored-by: Igor Zhukov <fsb4000@yandex.ru>
miscco
miscco commented Jul 2, 2021
View reviewed changes
stl/inc/istream Outdated Show resolved Hide resolved
miscco
miscco commented Jul 2, 2021
View reviewed changes
stl/inc/istream Show resolved Hide resolved
@miscco @AdamBucior
Properly properly catch exceptions
201c4cf 
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
CaseyCarter
CaseyCarter approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

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

These are more suggestions than required changes, so I'll go ahead and approve.

tests/std/tests/GH_001858_iostream_exception/test.cpp Show resolved Hide resolved
stl/inc/istream Show resolved Hide resolved
stl/inc/istream Show resolved Hide resolved
stl/inc/ostream Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Aug 24, 2021
StephanTLavavej
StephanTLavavej requested changes Aug 25, 2021
View reviewed changes
stl/inc/istream Show resolved Hide resolved
stl/inc/istream Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Aug 25, 2021
@StephanTLavavej StephanTLavavej removed their assignment Aug 26, 2021
@miscco
Copy link
Contributor Author

miscco commented Aug 26, 2021

From my point of view the current iostream situation is a bit broken. We do a lot of gymnastics to not call setstate inside a catch block when it does not really bring any benefit due to the _Reraise = true case that we trigger when the catch block triggers.

I think we could even get around the double exception it if we would simply call setstate(..., true) in those cases, as that would immediately just throw rather than creating an exception that is then eaten by the _CATCH_IO_END

That said, I would definitely not touch this in this PR

StephanTLavavej
StephanTLavavej reviewed Aug 30, 2021
View reviewed changes
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, I'll go ahead and push changes for one last round of feedback.

tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

Sign in to view
@StephanTLavavej StephanTLavavej self-assigned this Sep 2, 2021
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 671daf4 into microsoft:main Sep 2, 2021
@StephanTLavavej
Copy link
Member

Thanks for fixing these EH bugs! 🐞 🦋 🎉

@miscco miscco deleted the iostream_exceptions branch September 2, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@fsb4000 fsb4000 fsb4000 left review comments

@AdamBucior AdamBucior AdamBucior left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<istream>/<ostream> : seekg() and seekp() must catch and rethrow internal io exceptions
5 participants
@miscco @StephanTLavavej @CaseyCarter @fsb4000 @AdamBucior