Skip to content

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

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
iritkatriel merged 4 commits into from
Jan 25, 2023
Merged

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

iritkatriel merged 4 commits into from
Jan 25, 2023

Conversation

@iritkatriel
Copy link
Member

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

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

iritkatriel commented Jan 25, 2023

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
@gvanrossum
Copy link
Member

gvanrossum commented Jan 25, 2023

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
Copy link
Member

@gvanrossumgvanrossum left a comment

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.

Tools/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();")
Copy link
Member

@gvanrossumgvanrossumJan 25, 2023

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.

Copy link
MemberAuthor

@iritkatrieliritkatrielJan 25, 2023

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.

Copy link
Member

@gvanrossumgvanrossumJan 25, 2023

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.

@iritkatrieliritkatriel 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)
@iritkatrieliritkatriel 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