Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-103092: Port some _ctypes data types to heap types#113630
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
neonene commented Jan 1, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
neonene commented Jan 2, 2024
@vstinner Could you review this behavior change? |
_ctypes data types to heap typesneonene commented Jan 5, 2024
This is an attempt without hacking type slots. Problem: It is possible that accessing metaclasses cause a crash with bad arguments (e.g. cc PEP 687 experts: @erlend-aasland@kumaraditya303 |
neonene commented Jan 6, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
encukou commented Jan 8, 2024
To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the IMO, the correct way with the current heap type creation API is to move the initialization to Another option is to improve the type creation API -- e.g. design a slot that works like |
vstinner commented Jan 8, 2024
I would prefer a smaller PR. You moved 7 types. Can you start with a PR which change less types and less files at once? |
neonene commented Jan 10, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@encukou Is it worth opening a new issue for that? Maybe a custom meta |
encukou commented Jan 10, 2024
Oh, I think it's worth writing a PEP for that :) |
neonene commented Jan 10, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Sorry, I'm not qualified, as I'm volunteering anonymously. |
vstinner commented Jan 19, 2024
Why do you think that a PEP is needed for this specific change? Many static types were converted to heap types in the stdlib without a specific PEP. |
erlend-aasland commented Jan 19, 2024
I think@encukou meant a PEP for this proposed idea in #113630 (comment) :)
|
vstinner commented Jan 19, 2024
How are ctypes types than other stdlib extensions types? Is there a metaclass? Why do they need special code for |
erlend-aasland commented Jan 19, 2024
|
vstinner commented Jan 19, 2024
Ah,
/* PyCStructType_Type - a meta type/class. Creating a new class using this one as __metaclass__ will call the constructor StructUnionType_new/init. It replaces the tp_dict member with a new instance of StgDict, and initializes the C accessible fields somehow.*/Extracts: and: |
erlend-aasland commented Jan 19, 2024
+1 for a dedicated issue; there is a lot of issues connected to the isolation of |
vstinner commented Jan 19, 2024
I created the issue gh-114314 "Convert _ctypes PyCStgDict meta static type to a meta heap type". |
vstinner commented Jan 19, 2024
I wrote PR #114316 for that. It's partially based on this PR. |
neonene commented Jan 20, 2024
Unlike this PR, the issue #114314 seems to consider how to refactor the hacks around PyCStgDict, without customizing Any way is fine with me to avoid the error related to #103968: |
This comment was marked as outdated.
This comment was marked as outdated.
neonene commented Mar 13, 2024
Closing the draft in favor of #116458. |
Currently,
PyType_From*functions refuse or ignore creating classes whose metaclass overridestp_new(#103968):The ctypes extension needs some workaround for that.
UPDATE: Added
TypeErrormessage above.