-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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-94673: Add Per-Interpreter tp_subclasses for Static Builtin Types #95301
gh-94673: Add Per-Interpreter tp_subclasses for Static Builtin Types #95301
Conversation
ericsnowcurrently
commented
Jul 26, 2022
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: [subinterpreters] Static types incorrectly share some objects between interpreters #94673
This reverts commit 0046b57446a26114fdf33d11a31a0fc84e7a764a.
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 2fa815e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
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.
LGTM, I have left some questions.
FWIW, we could also get rid of the new type flag and use tp_subclasses
as a tagged pointer and steal one bit to check if the tp_subclasses
is per interpreter or local, I'll probably do that but for now this is fine.
I'm afraid this isn't backwards compatible. The Consider:
|
FWIW, |
The current implementation uses a dictionary to store subclasses as a mapping of
Why do you expect it to raise an exception when you dereference a pointer which is clearly documented to be a internal implementation detail? |
I'm not saying this is a bad change. I'm asking to be mindful of the possible effects on code other than CPython that might look at this field.
Yes, the docs are wrong. I don't think that's a free pass make this an invalid
Well, I do expect to be able to inspect internal details, e.g. for debugging. |
Hey, the feedback is super helpful! Thanks! I'll go ahead and add a whatsnew entry about the special-casing of |
Making |
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit dace9c4 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Thanks! |
- Member documentation describes the current state, for users. - The ``versionchanged`` note describes the change. - The What's New entry is, ideally, usable for someone stuck with maintaining a mysterious legacy codebase that stopped working. Details of internals are left out -- if it isn't enough to look at the source, they should be documented in the devguide rather than user-facing docs.