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

Remove noexcept from coroutine_handle<>::resume and operator() #1182

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

joemmett
Copy link
Member

With the adoption of coroutine issue 25
it is no longer undefined behavior for an exception to escape the body
of a coroutine, e.g. by rethrowing from unhandled_exception. Compiler
support for handling such an exception was introduced to MSVC with
version 16.7, and with this support it is now well-defined for resume
to exit with an exception. This PR removes the strengthened noexcept
specifiers from both resume and operator() which previously trapped
the undefined behavior.

With the adoption of [coroutine issue 25](http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p0664r6.html#25)
it is no longer undefined behavior for an exception to escape the body
of a coroutine, e.g. by rethrowing from `unhandled_exception`. Compiler
support for handling such an exception was introduced to MSVC with
version 16.7, and with this support it is now well-defined for `resume`
to exit with an exception. This PR removes the strengthened noexcept
specifiers from both `resume` and `operator()` which previously trapped
the undefined behavior.
@joemmett joemmett requested a review from a team as a code owner August 11, 2020 17:44
@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 11, 2020
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.

I wonder whether we should have comments along the lines of "must not be strengthened", but perhaps the eventual addition of test coverage will cover this. In any event, this has already been merged in MSVC.

@StephanTLavavej StephanTLavavej merged commit 1680ae7 into microsoft:master Aug 11, 2020
@StephanTLavavej
Copy link
Member

Thanks for this exceptional improvement! 😎

@CaseyCarter
Copy link
Member

Thanks for this exceptional improvement! 😎

Thanks for this amazing co_ntribution!

@joemmett joemmett deleted the resume_noexcept branch March 11, 2021 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants