Skip to content

gh-98831: add variable stack effect support to cases generator - #101309

Merged
iritkatriel merged 4 commits into
python:mainfrom
iritkatriel:metadata
Jan 25, 2023
Merged

gh-98831: add variable stack effect support to cases generator#101309
iritkatriel merged 4 commits into
python:mainfrom
iritkatriel:metadata

Conversation

@iritkatriel

@iritkatrieliritkatriel commented Jan 25, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@bedevere-botbedevere-bot mentioned this pull request Jan 25, 2023
Closed
@iritkatriel

iritkatriel commented Jan 25, 2023

Copy link
Copy Markdown
MemberAuthor

We’ll want something more efficient than this. Maybe generate a function with a switch statement for each value?

gvanrossum
gvanrossum reviewed Jan 25, 2023
View reviewed changes
Comment threadPython/opcode_metadata.h Outdated
@gvanrossum

gvanrossum commented Jan 25, 2023

Copy link
Copy Markdown
Member

We’ll want something more efficient than this. Maybe generate a function with a switch statement for each value?

Yes. Read this after writing my response. :-)

gvanrossum
gvanrossum reviewed Jan 25, 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.

Very nice, just one request and one suggestion.

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py
self.out.emit(f" case {instr.name}:")
self.out.emit(f" return {effect};")
self.out.emit(" default:")
self.out.emit(" Py_UNREACHABLE();")

@gvanrossumgvanrossumJan 25, 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.

If you make this return -1; then you can drop all the cases that return -1, making the function much shorter.

@iritkatrieliritkatrielJan 25, 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.

There won't be any -1's when we finish. We could group all cases with the same value if we want.

@gvanrossumgvanrossumJan 25, 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.

Ah, makes sense. LGTM then.

@iritkatriel
iritkatriel merged commit 19f90d6 into python:mainJan 25, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this pull request Jan 25, 2023
@iritkatriel
pythongh-98831: add variable stack effect support to cases generator (p…
b55a4a3
…ython#101309)
mdboom pushed a commit to mdboom/cpython that referenced this pull request Jan 31, 2023
@iritkatriel@mdboom
pythongh-98831: add variable stack effect support to cases generator (p…
e5c2f55
…ython#101309)
@iritkatriel
iritkatriel deleted the metadata branch April 3, 2023 17:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iritkatriel@gvanrossum@bedevere-bot