Skip to content

bpo-46202: Remove opcode POP_EXCEPT_AND_RERAISE#30302

Merged
markshannon merged 10 commits into
python:mainfrom
iritkatriel:remove-POP_EXCEPT_AND_RERAISE
Jan 4, 2022
Merged

bpo-46202: Remove opcode POP_EXCEPT_AND_RERAISE#30302
markshannon merged 10 commits into
python:mainfrom
iritkatriel:remove-POP_EXCEPT_AND_RERAISE

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Dec 30, 2021

Copy link
Copy Markdown
Member

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

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

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:

Comment thread Python/compile.c Outdated
@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@iritkatriel

Copy link
Copy Markdown
Member Author

I think the bot wants this in a new comment:

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

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

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

Comment thread Python/ceval.c
Comment thread Python/compile.c Outdated
Comment thread Python/compile.c Outdated
Comment thread Python/compile.c
Comment thread Python/ceval.c
@iritkatriel

Copy link
Copy Markdown
Member Author

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.

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

LGTM

(assuming you address any other review comments, of course)

@iritkatriel

Copy link
Copy Markdown
Member Author

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.

@iritkatriel iritkatriel marked this pull request as ready for review January 2, 2022 23:36
@iritkatriel

Copy link
Copy Markdown
Member Author

I committed the except* changes separately so this PR now just removes POP_EXCEPT_AND_RERAISE.

Comment thread Misc/NEWS.d/next/Core and Builtins/2021-12-30-11-06-27.bpo-46202.IKx4v6.rst Outdated

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

LGTM

@markshannon markshannon merged commit a94461d into python:main Jan 4, 2022
@iritkatriel iritkatriel deleted the remove-POP_EXCEPT_AND_RERAISE branch January 13, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants