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-102021 : Allow multiple input files for interpreter loop generator by jbower-fb · Pull Request #102022 · python/cpython

Conversation

@jbower-fb

@jbower-fb jbower-fb commented Feb 18, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I'm not sure a NEWS blurb is needed as the generator is entirely new in 3.12 anyway.

  • Issue: Enable definition for generated interpreter cases to be composed from multiple files #102021

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added the awaiting review label Feb 18, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 18, 2023
Enable definition for generated interpreter cases to be composed from multiple files #102021
Closed
@bedevere-bot

bedevere-bot commented Feb 18, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

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

@arhadthedev arhadthedev added the skip news label Feb 18, 2023
@arhadthedev

arhadthedev commented Feb 18, 2023

Copy link
Copy Markdown
Member

@gvanrossum @iritkatriel as authors of cases_generator.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Feb 18, 2023

Copy link
Copy Markdown
Member

I would like to wait reviewing until I am back from my break, around 2/27.

jbower-fb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@gvanrossum
gvanrossum self-requested a review February 28, 2023 22:55
@gvanrossum gvanrossum self-assigned this Feb 28, 2023
gvanrossum
gvanrossum reviewed Mar 1, 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.

Thanks for doing this! I have a few suggestions, feel free to push back!

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 Outdated
families: dict[str, parser.Family]

def every_thing(self) -> Iterable[parser.InstDef | parser.Super | parser.Macro]:
return itertools.chain(

@gvanrossum gvanrossum Feb 28, 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.

I'm not a big fan of itertools.chain. Maybe make this a generator?

I also noticed this is causing the output to be emitted in a different order again. But I quite like having the generated instructions in the same order as they occur in the input. Could you restore that behavior? (I understand that it's a little tricky due to overrides. Ideally there would be a comment left in the output for instructions that are overridden by later ones.)

Sorry, something went wrong.

Uh oh!

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

jbower-fb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@jbower-fb jbower-fb Mar 3, 2023

Copy link
Copy Markdown
Contributor 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.

Okay, I've changed things now so the output will retain the definition order and if there are overrides they'll look something like this:

...
        // TARGET(NOP) overridden by later definition
...
        // Override
        TARGET(NOP) {
            DISPATCH();

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 Resolved

Uh oh!

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

Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

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

jbower-fb added 2 commits March 3, 2023 12:32
@jbower-fb
Allow multiple input files for interpreter cases generator
86c0ca1
@jbower-fb
Address review comments
c8495cd
@jbower-fb
jbower-fb force-pushed the multiple-cases branch from d8c3165 to c8495cd Compare March 3, 2023 20:58
@gvanrossum

gvanrossum commented Mar 3, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

(In the future please don't force push. It makes things more complicated for the reviewer, and we squash commits at the end anyway.)

jbower-fb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

gvanrossum
gvanrossum approved these changes Mar 3, 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.

LG!

I'll apply the small correction to the metadata header comment, wait for tests to pass again, and merge it. Thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/opcode_metadata.h Resolved

Uh oh!

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

Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 3, 2023
@gvanrossum
Don't mention --metadata in header comment
a46d656
@jbower-fb

jbower-fb commented Mar 3, 2023

Copy link
Copy Markdown
Contributor Author

Great, thank you.

All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum
gvanrossum reviewed Mar 3, 2023
View reviewed changes
Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

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

@gvanrossum
Typo
9efb929
@gvanrossum
gvanrossum merged commit 8de59c1 into python:main Mar 4, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 4, 2023
carljm added a commit to carljm/cpython that referenced this pull request Mar 4, 2023
@carljm
Merge branch 'main' into nodeaddict
a31af32
* main:
  pythongh-102021 : Allow multiple input files for interpreter loop generator (python#102022)
  Add import of `unittest.mock.Mock` in documentation (python#102346)
  pythongh-102383: [docs] Arguments of `PyObject_CopyData` are `PyObject *` (python#102390)
  pythongh-101754: Document that Windows converts keys in `os.environ` to uppercase (pythonGH-101840)
  pythongh-102324: Improve tests of `typing.override` (python#102325)
hugovk pushed a commit to hugovk/cpython that referenced this pull request Mar 6, 2023
@jbower-fb @hugovk
pythongh-102021 : Allow multiple input files for interpreter loop gen…
46488eb
…erator (python#102022)

The input files no longer use `-i`.
@jbower-fb
jbower-fb deleted the multiple-cases branch May 1, 2023 22:07
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

Uh oh!

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

Assignees

@gvanrossum gvanrossum

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.

4 participants

@jbower-fb @bedevere-bot @arhadthedev @gvanrossum
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.