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-105481: Mark more files as generated#107598

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
brandtbucher merged 1 commit intopython:mainfrombrandtbucher:generated-files
Aug 4, 2023

Conversation

brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commentedAug 3, 2023
edited by bedevere-bot
Loading

@@ -72,9 +72,11 @@ Doc/library/token-list.inc generated
Include/internal/pycore_ast.h generated
Include/internal/pycore_ast_state.h generated
Include/internal/pycore_opcode.h generated
Include/internal/pycore_opcode_metadata.h generated
Copy link
Member

Choose a reason for hiding this comment

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

I removed this from the list a couple of weeks ago to make it show up in the diff, to make sure people are aware when the instruction flags change.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, let’s keep it out of the list for. This can catch accidental mistakes, or remind people of the consequences of intentional changes.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should leave it there commented out with an explanation so this won't come up again.

Copy link
MemberAuthor

@brandtbucherbrandtbucherAug 3, 2023
edited
Loading

Choose a reason for hiding this comment

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

Hm, really? I'm not sure I agree. This was prompted by the diff view ofmy recent PR, where it just adds noise.

We already hide other files with lists of tokens, keywords, and standard library module names that are arguably more important and more human-readable than this one. And people can still view them, GitHub just collapses them by default.

If we're this concerned about setting the correct flags, maybe we should just write some sort of test instead?

Copy link
Member

Choose a reason for hiding this comment

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

What test would you write?

Copy link
MemberAuthor

@brandtbucherbrandtbucherAug 3, 2023
edited
Loading

Choose a reason for hiding this comment

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

Well, I'm not really sure what we're trying to protect against here. My understanding is that the flags are now generated automatically by analyzing the DSL's C code. If there's a bug somewhere, it's probably in the static analysis of the code, not the code itself (if I add aGETLOCAL to something, then I definitely want it to set the has "local" flag, ditto forJUMPBY and the "jump" flag).

So maybe tests forInstructionFlags.fromInstruction with some expected inputs and outputs? Or some error-checking in the generator that incompatible flag combinations (like "local" and "jump", or "const" without "oparg") don't happen?

Copy link
Member

Choose a reason for hiding this comment

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

We could have a few of those. They would protect us from regressions when we change the static analysis code, or when we change the implementation of a bytecode that happens to be tested. But when you add a new bytecode, don't you want to look at that flags and see that they make sense?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

(I edited my comment above, adding that maybe the flags class could do some sanity checks on the flag combination.)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

But honestly I thought this would be uncontroversial. If you're getting value from the status quo, I can let it go. I just got kind of tired of scrolling past this file in PRs.

Copy link
Member

@gvanrossumgvanrossum left a comment
edited
Loading

Choose a reason for hiding this comment

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

Thinking more about this I think this PR is actually fine and we're just being overly paranoid. When changing the code generator I review all generated output anyways, and ditto when editing or adding bytecodes. But I'll leave the last word to@iritkatriel

@iritkatriel
Copy link
Member

Ok, we can try it this way.

@brandtbucherbrandtbucher merged commite52e87c intopython:mainAug 4, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@iritkatrieliritkatrieliritkatriel approved these changes

@gvanrossumgvanrossumgvanrossum approved these changes

Assignees

@brandtbucherbrandtbucher

Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@brandtbucher@iritkatriel@gvanrossum@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp