Skip to content

Conversation

@markshannon
Copy link
Member

@markshannonmarkshannon commented Nov 21, 2023

@gvanrossumgvanrossum changed the title Seperate out parsing, analysis and code-gen phasesSeparate out parsing, analysis and code-gen phasesNov 21, 2023
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.

It's more of a rewrite (or everything besides the parser and lexer) than a refactor. :-) It does feel cleaner, so I support forging ahead here.

General: I'd like it to use argparse, have correct type annotations everywhere, and be formatted using Black.

I'd like to understand why _PUSH_FRAME and the macros using it are now considered not to have an output stack effect (or one less, for the macros). Given that the generated output is unchanged for these, I am assuming that you're no longer checking that all the members of a family have the same stack effect?

One concern I have with creating separate top-level command-line utilities is that in my experience, the bulk of the processing time is in the parser -- which means that if you have five utilities to generate each of the five output files, you end up waiting five times as long for make regen-cases or its equivalent. This should be easy to address given your architecture. Please don't forget to update README.md.

I didn't try to understand your stack.py carefully, but it deserves a lot of scrutiny. In the past fixes there usually were obvious when the generated code was different -- but you've moved things around in the output just enough (even taking the case-sorting out of the equation) that it's hard to review everything without glazing over.

@markshannonmarkshannon changed the title Separate out parsing, analysis and code-gen phasesGH-111485: Separate out parsing, analysis and code-gen phasesNov 22, 2023
@markshannonmarkshannon marked this pull request as ready for review December 1, 2023 11:32
returnself._size


Part=Uop|Skip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skip is "unused cache entry", but here it's used as a uop?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is Part used as a Uop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uops: list[Part] in line 133

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a list of parts, and used as such. I'll change the name.

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some optional suggestions:

Comment on lines +14 to +15
allow_redefinition = True
implicit_reexport = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: move these lines of the config file above the # ...And be strict: comment on line 9, since they don't relate to increasing the strictness of mypy

fromtypingimportOptional


@dataclass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be tempted to do this, for readability. I agree with Guido's opinion that it's hard to read when instances of this class are constructed using positional arguments

Suggested change
@dataclass
@dataclass(kw_only=True)

@markshannonmarkshannon merged commit b449415 into python:mainDec 7, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@markshannonmarkshannon deleted the refactor-code-gen branch August 6, 2024 10:18
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@markshannon@gvanrossum@iritkatriel@AlexWaygood