Skip to content

gh-109256: allocate opcode IDs for internal opcodes in their own range#109269

Merged
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:internal_ops
Sep 12, 2023
Merged

gh-109256: allocate opcode IDs for internal opcodes in their own range#109269
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:internal_ops

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Sep 11, 2023 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@iritkatriel iritkatriel changed the title gh-09256: allocate opcode IDs for internal opcodes in their own range gh-109256: allocate opcode IDs for internal opcodes in their own range Sep 11, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Sep 11, 2023
Closed
markshannon
markshannon reviewed Sep 11, 2023
View reviewed changes

@markshannon markshannon left a comment •
edited
Loading

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.

Looks good, in general.

test_dis will need fixing up, of course.

As you need to fix test_dis anyway could you make RESUME opcode 1? If it is to have a special number, it might as well be memorable.

    map_op(1, "RESUME")
    next_opcode = 2

@iritkatriel

iritkatriel commented Sep 11, 2023

Copy link
Copy Markdown
Member Author

Looks good, in general.

test_dis will need fixing up, of course.

As you need to fix test_dis anyway could you make RESUME opcode 1? If it is to have a special number, it might as well be memorable.

    map_op(1, "RESUME")
    next_opcode = 2

RESUME has an arg though, so I thought it should be after HAVE_ARGUMENT? Unless we want to special case it.

@brettcannon
brettcannon removed their request for review September 11, 2023 22:48
@markshannon
markshannon self-requested a review September 12, 2023 08:26
markshannon
markshannon approved these changes Sep 12, 2023
View reviewed changes

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

Looks good. Should save a fair bit of future churn.
Merge conflicts need fixing, obviously.

@markshannon markshannon mentioned this pull request Sep 12, 2023
Closed
@iritkatriel
iritkatriel enabled auto-merge (squash) September 12, 2023 10:12
@iritkatriel
iritkatriel merged commit 8b55adf into python:main Sep 12, 2023
@gvanrossum gvanrossum mentioned this pull request Sep 12, 2023
Merged
vstinner pushed a commit to vstinner/cpython that referenced this pull request Sep 13, 2023
@iritkatriel @vstinner
pythongh-109256: allocate opcode IDs for internal opcodes in their ow…
547afcc 
…n range (python#109269)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iritkatriel @markshannon