Skip to content
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-98831: Modernize FORMAT_VALUE #101628

Merged
merged 4 commits into from
Feb 8, 2023
Merged

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 7, 2023

This was the simplest one I've done in a while, except that I had to fix the DSL parser to support balanced parentheses and brackets in conditions (and array sizes, as a side benefit).

@gvanrossum
Copy link
Member Author

This wasn't simple after all. A test crashed because there was an error path with a manual goto error when the inputs had already been decref'ed. This caused a reference deficiency for the inputs on the stack leading to a crash that had me stumped until I picked up paper and pencil and reasoned through the generated code.

While I think I've fixed this now, this points out a general weakness in our bytecode definitions. Sometimes we feel forced to leave a manual goto error in the code (though note that in this case it was an oversight, for which I take full responsibility), but maintaining reference count correctness in all cases is hard!

@gvanrossum gvanrossum marked this pull request as ready for review February 7, 2023 18:37
@gvanrossum gvanrossum added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 7, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 1e48f9d 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 7, 2023
@gvanrossum
Copy link
Member Author

Ready for review.

@gvanrossum
Copy link
Member Author

gvanrossum commented Feb 7, 2023

This is blocked until I can figure out the refleak failures. (EDIT: But first I'm going to re-run that particular build, in case it's a flake.)

@gvanrossum
Copy link
Member Author

Hm, aarch64 RHEL8 Refleaks PR seems to have been failing a lot, historically. Ignoring.

@gvanrossum gvanrossum merged commit b2b85b5 into python:main Feb 8, 2023
@gvanrossum gvanrossum deleted the format-value branch February 8, 2023 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants