Skip to content

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

Merged
Stephan T. Lavavej (StephanTLavavej) merged 16 commits into
microsoft:mainfrom
miscco:iostream_exceptions
Sep 2, 2021
Merged

Properly catch exceptions in {i|o}stream#2033
Stephan T. Lavavej (StephanTLavavej) merged 16 commits into
microsoft:mainfrom
miscco:iostream_exceptions

Conversation

@miscco

@miscco Michael Schellenberger Costa (miscco) commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

Fixes #1858

fsb4000
Igor Zhukov (fsb4000) reviewed Jul 2, 2021
View reviewed changes
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
@StephanTLavavej Stephan T. Lavavej (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
Michael Schellenberger Costa (miscco) commented Jul 2, 2021
View reviewed changes
Comment thread stl/inc/istream Outdated
miscco
Michael Schellenberger Costa (miscco) commented Jul 2, 2021
View reviewed changes
Comment thread stl/inc/istream
@miscco @AdamBucior
Properly properly catch exceptions
201c4cf 
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
CaseyCarter
Casey Carter (CaseyCarter) approved these changes Aug 18, 2021
View reviewed changes

@CaseyCarter Casey Carter (CaseyCarter) left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp
Comment thread stl/inc/istream
Comment thread stl/inc/istream
Comment thread stl/inc/ostream Outdated
@CaseyCarter Casey Carter (CaseyCarter) removed their assignment Aug 24, 2021
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Aug 25, 2021
View reviewed changes
Comment thread stl/inc/istream
Comment thread stl/inc/istream Outdated
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Aug 25, 2021
View reviewed changes
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
@miscco

Michael Schellenberger Costa (miscco) commented Aug 26, 2021

Copy link
Copy Markdown
Contributor Author

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
Stephan T. Lavavej (StephanTLavavej) reviewed Aug 30, 2021
View reviewed changes

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) left a comment

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.

Looks good, I'll go ahead and push changes for one last round of feedback.

Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp
Comment thread tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated
@StephanTLavavej

This comment has been minimized.

Sign in to view
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Sep 2, 2021

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 671daf4 into microsoft:main Sep 2, 2021
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Sep 2, 2021

Copy link
Copy Markdown
Member

Thanks for fixing these EH bugs! 🐞 🦋 🎉

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

Reviewers

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+3 more reviewers
@CaseyCarter Casey Carter (CaseyCarter) CaseyCarter approved these changes
@fsb4000 Igor Zhukov (fsb4000) fsb4000 left review comments
@AdamBucior Adam Bucior (AdamBucior) AdamBucior left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej Stephan T. Lavavej (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