GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-98831: add variable stack effect support to cases generator by iritkatriel · Pull Request #101309 · python/cpython

Conversation

@iritkatriel

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iritkatriel
pythongh-98831: add variable stack effect support to cases generator
1fbf963
@iritkatriel
iritkatriel requested a review from gvanrossum January 25, 2023 00:43
@iritkatriel
iritkatriel requested a review from markshannon as a code owner January 25, 2023 00:43
@bedevere-bot bedevere-bot added the awaiting core review label Jan 25, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 25, 2023
Generate the interpreter #98831
Closed
@iritkatriel

iritkatriel commented Jan 25, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum reviewed Jan 25, 2023
View reviewed changes
Comment thread Python/opcode_metadata.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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. :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel added 2 commits January 25, 2023 14:24
@iritkatriel
stack effect refactored into separate functions with switch
205a1a9
@iritkatriel
_PyOpcode_opcode_metadata back to static array
0ccbabd
@iritkatriel iritkatriel added the skip news label Jan 25, 2023
gvanrossum
gvanrossum reviewed Jan 25, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread 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();")

@gvanrossum gvanrossum Jan 25, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iritkatriel iritkatriel Jan 25, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Jan 25, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iritkatriel
type annotations
9a71113
gvanrossum
gvanrossum approved these changes Jan 25, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jan 25, 2023
@iritkatriel
iritkatriel merged commit 19f90d6 into python:main Jan 25, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@iritkatriel @gvanrossum @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.