-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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.
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
gh-123881: make compiler add the .generic_base base class without constructing AST nodes #123883
Conversation
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 feel this is worse, since it makes the AST returned by ast.parse
different from what users actually see. Users dealing with the AST will have to learn about this new oddity.
The issue this is attached to says the compiler modifies the AST, but it doesn't; it creates a new ASDL seq and inserts an extra entry into it. The original AST is not modified.
When you're done making the requested changes, leave the comment: |
This is true anyway, the AST currently gives no indication that |
Python/codegen.c
Outdated
RETURN_IF_ERROR_IN_SCOPE(c, codegen_call_helper(c, loc, 2, | ||
bases, | ||
s->v.ClassDef.bases, |
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 it's important to you to remove the code creating a fake AST node here, you could instead add an extra argument to codegen_call_helper
that is usually NULL and maps to an extra argument to add at the end of the list. I think I did that initially while working on PEP 695, but it seemed worse than creating a new virtual asdl_seq as the current code does. (Possibly you were involved in discussing that in person, don't remember exactly.)
But I'd like to keep the extra base class as an implementation detail in the compiler, not something that gets exposed in the AST layer.
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.
Ok, I'll try something like that, should be quite simple.
It's possible I was involved in the discussion at the time, I don't remember the details.
I think it would be better if the compiler didn't need to know how to create AST nodes. (I take your point about it not modifying the AST, I'll correct the wording in the issue)
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 found back the discussion from last time: #103764 (comment).
I have made the requested changes; please review again. |
Thanks for making the requested changes! @JelleZijlstra: please review the changes made to this pull request. |
Fixes #123881.