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-90928: Statically Initialize the Keywords Tuple in Clinic-Generated Code by ericsnowcurrently · Pull Request #95860 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Aug 10, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

We only statically initialize for core code and builtin modules. Extension modules still create the tuple at runtime. We'll solve that part of interpreter isolation separately.

This change includes generated code. The significant changes are in:

  • Tools/clinic/clinic.py
  • Python/getargs.c
  • Include/cpython/modsupport.h
  • Makefile.pre.in (re-generate global strings after running clinic)
  • very minor tweaks to Modules/_codecsmodule.c and Python/Python-tokenize.c

All other changes are generated code (clinic, global strings).

  • Issue: Statically Initialize PyArg_Parser in clinic.py #90928

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba, kumaraditya303, and erlend-aasland reacted with hooray emoji erlend-aasland and kumaraditya303 reacted with rocket emoji
All reactions
  • 🎉 3 reactions
  • 🚀 2 reactions
ericsnowcurrently and others added 14 commits August 10, 2022 11:48
@ericsnowcurrently
Do not squash the _PyArg_Parser declarations into a single line.
e03f6cd
@ericsnowcurrently
Factor out declare_parser().
3737e79
@ericsnowcurrently
Simplify.
f022fcc
@ericsnowcurrently
Make it clear about when format is NULL.
cfee830
@ericsnowcurrently
Factor out scan_keywords().
4b87250
@ericsnowcurrently
Factor out parse_format().
a52d83f
@ericsnowcurrently
Factor out new_kwtuple().
33227de
@ericsnowcurrently
Add an explicit "initialized" field.
698c35a
@ericsnowcurrently
Statically initialize PyArg_Parser.kwtuple.
7c7f497
@ericsnowcurrently
Explicitly import some internal headers.
22b8a20
@ericsnowcurrently
Do not statically init kwtuple for Py_BUILD_CORE_MODULE.
b9312d0
@kumaraditya303 @ericsnowcurrently
fix hashing
d1287d0
@ericsnowcurrently
Drop all generated code (temporarily).
4deab96
@ericsnowcurrently
Re-generate.
92e78fe
@ericsnowcurrently ericsnowcurrently added the skip news label Aug 10, 2022
@ericsnowcurrently
ericsnowcurrently requested a review from kumaraditya303 August 10, 2022 20:24
@ericsnowcurrently
ericsnowcurrently requested review from a team, 1st1, Fidget-Spinner, JelleZijlstra, abalkin, asvetlov, berkerpeksag, erlend-aasland, ethanfurman, gvanrossum, pganssle, rhettinger and tiran as code owners August 10, 2022 20:24
@bedevere-bot bedevere-bot added the awaiting core review label Aug 10, 2022
@ericsnowcurrently
ericsnowcurrently removed request for Fidget-Spinner, erlend-aasland, ethanfurman, gvanrossum, pganssle and rhettinger August 10, 2022 20:24
@ericsnowcurrently ericsnowcurrently mentioned this pull request Aug 10, 2022
Statically initialize _PyArg_Parser kumaraditya303/cpython#4
Closed
@ericsnowcurrently
ericsnowcurrently marked this pull request as draft August 10, 2022 20:37
ericsnowcurrently and others added 4 commits August 10, 2022 15:11
@ericsnowcurrently
Ignore positional-only args.
13e4b89
@ericsnowcurrently
Fix parser_init().
5eafa67
@kumaraditya303 @ericsnowcurrently
windows fix part 1
c90f0a8
@ericsnowcurrently
Update the clinic tests.
3b308c4
@ericsnowcurrently
ericsnowcurrently marked this pull request as ready for review August 10, 2022 22:55
erlend-aasland
erlend-aasland reviewed Aug 11, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

kumaraditya303 commented Aug 11, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

LGTM.

We only statically initialize for core code and builtin modules. Extension modules still create the tuple at runtime.

Yeah, its better to solve it for builtins now, extensions modules can be solved later on.
FTR, I tried solving both at once 1 and it worked on Linux but does not work on Windows because it does not allows taking addresses of DLL loaded symbols.

Footnotes

  1. https://github.com/kumaraditya303/cpython/pull/4 ↩
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 approved these changes Aug 11, 2022
View reviewed changes
@ericsnowcurrently

ericsnowcurrently commented Aug 11, 2022

Copy link
Copy Markdown
Member Author

Yeah, its better to solve it for builtins now, extensions modules can be solved later on.
FTR, I tried solving both at once 2 and it worked on Linux but does not work on Windows because it does not allows taking addresses of DLL loaded symbols.

I'm glad you agree. 😄 Splitting it up this way made the most sense.
kumaraditya303 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
Drop an extraneous edit.
2efabaa
@ericsnowcurrently ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2022
@bedevere-bot

bedevere-bot commented Aug 11, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 3b308c4 🤖

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2022
@ericsnowcurrently
Add a comment.
44a193b
kumaraditya303
kumaraditya303 approved these changes Aug 11, 2022
View reviewed changes
@erlend-aasland

erlend-aasland commented Aug 11, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

CC. @colorfulappl, regarding your PR #32092. This PR touches some of the same parts of AC and getarg.c.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently merged commit 6f6a4e6 into python:main Aug 11, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Aug 11, 2022
@ericsnowcurrently
ericsnowcurrently deleted the global-objects-pyargs-parser-keywords-init branch August 11, 2022 21:25
@erlend-aasland erlend-aasland mentioned this pull request Aug 11, 2022
gh-90928: Improve static initialization of keywords tuple in AC #95907
Merged
@erlend-aasland erlend-aasland mentioned this pull request Aug 16, 2023
gh-107704: Argument Clinic: add support for deprecating keyword use of parameters #107984
Merged
@ericsnowcurrently ericsnowcurrently mentioned this pull request May 22, 2024
getargs.c is Breaking Interpeter Isolation #119213
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Uh oh!

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

Assignees

No one assigned

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

@ericsnowcurrently @kumaraditya303 @bedevere-bot @erlend-aasland
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 01402520 — 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.