GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-105481: Mark more files as generated by brandtbucher · Pull Request #107598 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Aug 3, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Generate opcode metadata from bytecodes.c instead of opcode.py #105481

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@brandtbucher
Mark more files as generated
2ca21a3
@brandtbucher brandtbucher added the skip news label Aug 3, 2023
@brandtbucher
brandtbucher requested review from gvanrossum and iritkatriel August 3, 2023 18:30
@brandtbucher brandtbucher self-assigned this Aug 3, 2023
@bedevere-bot bedevere-bot mentioned this pull request Aug 3, 2023
Generate opcode metadata from bytecodes.c instead of opcode.py #105481
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Aug 3, 2023
iritkatriel
iritkatriel reviewed Aug 3, 2023
View reviewed changes
Comment thread .gitattributes
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

@iritkatriel iritkatriel Aug 3, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gvanrossum gvanrossum Aug 3, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Aug 3, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Aug 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

Hm, really? I'm not sure I agree. This was prompted by the diff view of my 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Aug 3, 2023

Copy link
Copy Markdown
Member

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.

What test would you write?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Aug 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

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 a GETLOCAL to something, then I definitely want it to set the has "local" flag, ditto for JUMPBY and the "jump" flag).

So maybe tests for InstructionFlags.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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Aug 3, 2023

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Aug 3, 2023

Copy link
Copy Markdown
Member Author

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 edited my comment above, adding that maybe the flags class could do some sanity checks on the flag combination.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Aug 3, 2023

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
gvanrossum
gvanrossum approved these changes Aug 3, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 3, 2023
@iritkatriel

iritkatriel commented Aug 4, 2023

Copy link
Copy Markdown
Member

Ok, we can try it this way.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel approved these changes Aug 4, 2023
View reviewed changes
@brandtbucher
brandtbucher merged commit e52e87c into python:main Aug 4, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@brandtbucher @iritkatriel @gvanrossum @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.