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-90928: Statically Initialize the Keywords Tuple in Clinic-Generated Code #95860

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Aug 10, 2022

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

@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review August 10, 2022 22:55
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Aug 11, 2022

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

@ericsnowcurrently
Copy link
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.

@ericsnowcurrently ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2022
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Aug 11, 2022

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

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.

4 participants