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

Jump to bottom

gh-94216: add pseudo instructions to the dis/opcodes modules #94241

Merged
merged 16 commits into from Jul 1, 2022
Merged

gh-94216: add pseudo instructions to the dis/opcodes modules #94241

merged 16 commits into from Jul 1, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jun 24, 2022 •
edited

Closes #94241.

@iritkatriel iritkatriel requested a review from markshannon as a code owner Jun 24, 2022
gvanrossum
gvanrossum reviewed Jun 27, 2022
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Just a few comments, not a full review (Brandt or Mark can do that)

Doc/library/dis.rst Outdated Show resolved Hide resolved
Doc/library/dis.rst Outdated Show resolved Hide resolved
@iritkatriel iritkatriel mentioned this pull request Jun 28, 2022
Open
markshannon
markshannon requested changes Jun 30, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

This should definitely help future testing.

Regarding "virtual opcode", I think I prefer the term pseudo-instruction, as used by RISC-V.
But that's for a future PR.

Doc/library/dis.rst Outdated Show resolved Hide resolved
Doc/library/dis.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 30, 2022

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

@iritkatriel iritkatriel changed the title gh-94216: add virtual opcodes to the dis/opcodes modules gh-94216: add pseudo instructions to the dis/opcodes modules Jun 30, 2022
@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Jun 30, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 30, 2022

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from markshannon Jun 30, 2022
markshannon
markshannon requested changes Jul 1, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

One nit, otherwise good.

Doc/library/dis.rst Outdated

.. opcode:: SETUP_WITH (target)

Like ``SETUP_FINALLY``, but in case of exception one more item is popped
Copy link
Member

@markshannon markshannon Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be SETUP_CLEANUP.
Or leave it as SETUP_FINALLY and add the part about lasti.

Copy link
Member Author

@iritkatriel iritkatriel Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes!

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 1, 2022

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

@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Jul 1, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 1, 2022

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from markshannon Jul 1, 2022
markshannon
markshannon approved these changes Jul 1, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

👍

@iritkatriel iritkatriel merged commit c57aad7 into python:main Jul 1, 2022
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum

@markshannon markshannon

@sweeneyde sweeneyde

@brandtbucher brandtbucher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@iritkatriel @bedevere-bot @gvanrossum @markshannon