Skip to content

gh-102021 : Allow multiple input files for interpreter loop generator - #102022

Merged
gvanrossum merged 4 commits into
python:mainfrom
jbower-fb:multiple-cases
Mar 4, 2023
Merged

gh-102021 : Allow multiple input files for interpreter loop generator#102022
gvanrossum merged 4 commits into
python:mainfrom
jbower-fb:multiple-cases

Conversation

@jbower-fb

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

Copy link
Copy Markdown
Contributor

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

@bedevere-botbedevere-bot mentioned this pull request Feb 18, 2023
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.

@arhadthedev

arhadthedev commented Feb 18, 2023

Copy link
Copy Markdown
Member

@gvanrossum@iritkatriel as authors of cases_generator.

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

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

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

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/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(

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

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.)

@jbower-fbjbower-fbMar 3, 2023

Copy link
Copy Markdown
ContributorAuthor

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();

Comment threadTools/cases_generator/generate_cases.py
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
@gvanrossum

gvanrossum commented Mar 3, 2023•
edited
Loading

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.)

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

LG!

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

Comment threadPython/opcode_metadata.h Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
@jbower-fb

jbower-fb commented Mar 3, 2023

Copy link
Copy Markdown
ContributorAuthor

Great, thank you.

gvanrossum
gvanrossum reviewed Mar 3, 2023
View reviewed changes
Comment threadTools/cases_generator/generate_cases.py Outdated
@gvanrossum
gvanrossum merged commit 8de59c1 into python:mainMar 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes

Assignees

@gvanrossumgvanrossum

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jbower-fb@bedevere-bot@arhadthedev@gvanrossum