Skip to content

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

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.

Sign up for GitHub

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
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
Loading

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

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

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

@iritkatriel iritkatriel mentioned this pull request Jun 28, 2022
Closed
markshannon
markshannon requested changes Jun 30, 2022
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

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.

@bedevere-bot
Copy link

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

I have made the requested changes; please review again.

@bedevere-bot
Copy link

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 June 30, 2022 18:57
markshannon
markshannon requested changes Jul 1, 2022
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

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

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

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

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

I have made the requested changes; please review again.

@bedevere-bot
Copy link

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 July 1, 2022 13:37
markshannon
markshannon approved these changes Jul 1, 2022
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

👍

@iritkatriel iritkatriel merged commit c57aad7 into python:main Jul 1, 2022
@iritkatriel iritkatriel deleted the virtual_opcodes branch September 29, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@markshannon markshannon markshannon approved these changes

@sweeneyde sweeneyde Awaiting requested review from sweeneyde

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

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

Successfully merging this pull request may close these issues.
4 participants
@iritkatriel @bedevere-bot @gvanrossum @markshannon