Skip to content

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

Merged
iritkatriel merged 16 commits intopython:mainfrom
iritkatriel:virtual_opcodes
Jul 1, 2022
Merged

gh-94216: add pseudo instructions to the dis/opcodes modules#94241
iritkatriel merged 16 commits intopython:mainfrom
iritkatriel:virtual_opcodes

Conversation

@iritkatriel
Copy link
Copy Markdown
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
Copy Markdown
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)

Comment thread Doc/library/dis.rst Outdated
Comment thread Doc/library/dis.rst Outdated
@iritkatriel iritkatriel mentioned this pull request Jun 28, 2022
Closed
markshannon
markshannon requested changes Jun 30, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread Doc/library/dis.rst Outdated
Comment thread Doc/library/dis.rst Outdated
Comment thread Doc/whatsnew/3.12.rst Outdated
@bedevere-bot
Copy link
Copy Markdown

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
Copy Markdown
Member Author

iritkatriel commented Jun 30, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

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

Comment thread Doc/library/dis.rst Outdated

.. opcode:: SETUP_WITH (target)

Like ``SETUP_FINALLY``, but in case of exception one more item is popped
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member Author

iritkatriel commented Jul 1, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

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