Skip to content

gh-105481: generate op IDs from bytecode.c instead of hard coding them in opcode.py - #107971

Merged
iritkatriel merged 18 commits into
python:mainfrom
iritkatriel:deopt
Aug 16, 2023
Merged

gh-105481: generate op IDs from bytecode.c instead of hard coding them in opcode.py#107971
iritkatriel merged 18 commits into
python:mainfrom
iritkatriel:deopt

Conversation

@iritkatriel

@iritkatrieliritkatriel commented Aug 15, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@bedevere-botbedevere-bot mentioned this pull request Aug 15, 2023
Closed
@iritkatriel
iritkatriel marked this pull request as draft August 15, 2023 12:01
@Eclips4Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Aug 15, 2023
ericsnowcurrently
ericsnowcurrently approved these changes Aug 15, 2023
View reviewed changes

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

Mostly LGTM. I've left a couple comments where there might be something further to be done.

(I'm approving under the assumption the comments will be resolved first, one way or another.)

Comment threadInclude/opcode_ids.h
Comment threadLib/opcode.py
@iritkatriel

iritkatriel commented Aug 15, 2023

Copy link
Copy Markdown
MemberAuthor

Mostly LGTM. I've left a couple comments where there might be something further to be done.

Thanks. A couple of tests for tracing/instrumentation are failing, I'm still debugging that.

@brettcannon
brettcannon removed their request for review August 15, 2023 21:57
@iritkatriel
iritkatriel marked this pull request as ready for review August 16, 2023 16:17
@iritkatriel
iritkatriel requested a review from a team as a code ownerAugust 16, 2023 16:17
gvanrossum
gvanrossum approved these changes Aug 16, 2023
View reviewed changes

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

The revolution! It's almost here!!

A few nits but no need to wait for a re-review.

Comment threadLib/test/test_dis.py
@@ -1617,197 +1616,197 @@ def _prepare_test_cases():
Instruction = dis.Instruction

expected_opinfo_outer = [

@gvanrossumgvanrossumAug 16, 2023

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 test is just preposterous. Let's commit to doing this in a different way in a future PR.

@iritkatrieliritkatrielAug 16, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@markshannon loves to hate this test too.

Comment threadTools/build/deepfreeze.py
Comment threadMakefile.pre.in
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
@iritkatriel
iritkatriel enabled auto-merge (squash) August 16, 2023 22:13
@iritkatriel
iritkatriel merged commit 665a439 into python:mainAug 16, 2023
@markshannonmarkshannon mentioned this pull request Sep 11, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently approved these changes
@gvanrossumgvanrossumgvanrossum approved these changes
@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner
@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iritkatriel@ericsnowcurrently@gvanrossum@bedevere-bot@Eclips4