-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
bpo-46202: Remove opcode POP_EXCEPT_AND_RERAISE #30302
bpo-46202: Remove opcode POP_EXCEPT_AND_RERAISE #30302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks good to me, except for one weird bug I stumbled upon while reviewing it.
It's not directly related to these changes, but it still probably deserves to be fixed:
When you're done making the requested changes, leave the comment: |
I think the bot wants this in a new comment: I have made the requested changes; please review again. |
Thanks for making the requested changes! @brandtbucher: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This strictly relates to re-raising an exception, right?
try:
...
except Exception:
...
raise # re-raise
and not raising an exception while handing another:
try:
...
except Exception as exc:
...
raise exc
# or raise Exception
# or raise exc from exc2
It's for both - it's when you are about the exit a finally block, and you have a previous "handled exception" on the stack (because this finally is from a try-except or a with-statement which is nested in an except clause), and above it you have the lasti and an exception that you need to raise. So you need to pop the TOS and the lasti below it and raise, but first you need to take the THIRD item and use it to restore exc_info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(assuming you address any other review comments, of course)
I think this PR lost focus a bit. I want to split the except* changes into a separate PR and just do a straightforward opcode removal in this one. |
I committed the except* changes separately so this PR now just removes POP_EXCEPT_AND_RERAISE. |
Misc/NEWS.d/next/Core and Builtins/2021-12-30-11-06-27.bpo-46202.IKx4v6.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Following the reduction of exc_info to just one stack item, POP_EXCEPT_AND_RERAISE can be replaced by an equivalent sequence of other opcodes.
One of the except* usages can be simplified further because it doesn't need to restore lasti.(this was moved to another pr).https://bugs.python.org/issue46202