Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-123881: make compiler add the .generic_base base class without constructing AST nodes#123883

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
iritkatriel merged 15 commits intopython:mainfromiritkatriel:generic
Sep 10, 2024

Conversation

@iritkatriel
Copy link
Member

@iritkatrieliritkatriel commentedSep 9, 2024
edited by bedevere-appbot
Loading

Copy link
Member

@JelleZijlstraJelleZijlstra left a 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 byast.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.

@bedevere-app
Copy link

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

@iritkatriel
Copy link
MemberAuthor

it makes the AST returned by ast.parse different from what users actually see.

This is true anyway, the AST currently gives no indication thatclass C[T]: pass has a base class.

asdl_seq_SET(bases,original_len,name_node);
RETURN_IF_ERROR_IN_SCOPE(c,codegen_call_helper(c,loc,2,
bases,
s->v.ClassDef.bases,
Copy link
Member

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

Copy link
MemberAuthor

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)

Copy link
Member

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

@iritkatriel
Copy link
MemberAuthor

I have made the requested changes; please review again.

@bedevere-app
Copy link

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

@iritkatrieliritkatriel added skip news interpreter-core(Objects, Python, Grammar, and Parser dirs) and removed awaiting change review labelsSep 9, 2024
@iritkatrieliritkatriel changed the titlegh-123881: add the .generic_base base class in the parser rather than the compilergh-123881: make compiler add the .generic_base base class without constructing AST nodesSep 10, 2024
@iritkatrieliritkatrielenabled auto-merge (squash)September 10, 2024 15:50
@iritkatrieliritkatriel merged commita2d0818 intopython:mainSep 10, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@jeremyhyltonjeremyhyltonAwaiting requested review from jeremyhylton

@isidenticalisidenticalAwaiting requested review from isidentical

@pablogsalpablogsalAwaiting requested review from pablogsal

@lysnikolaoulysnikolaouAwaiting requested review from lysnikolaou

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Compiler should not need to know how to construct AST nodes

2 participants

@iritkatriel@JelleZijlstra

[8]ページ先頭

©2009-2025 Movatter.jp