-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-100926: Move ctype's pointers cache to StgInfo #131282
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
base: main
Are you sure you want to change the base?
gh-100926: Move ctype's pointers cache to StgInfo #131282
Conversation
Co-authored-by: neonene <53406459+neonene@users.noreply.github.com>
Co-authored-by: neonene <53406459+neonene@users.noreply.github.com>
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.
IMO, this needs a NEWS blurb. I'd even add a What's New entry -- _pointer_type_cache
is nominally internal API, but it's been there for a long time and people are using it.
Thanks! I'll add both entries later today. |
I have added news and whatsnew entries, but I believe we should update them after we decided what to do with _ctypes_ptrtype_cache behaviour. |
As far as I can see, this numeric ID key is only used for a check, and I think that it's OK to remove a check for obsolete functionality from a deprecated function. The feature is "incomplete pointers" -- ones that were created with a name, but weren't assigned an underlying type yet. See What I'm a bit worried about is the duplication between Should we expose the pointer type as a Python attribute? class PointerTypeCache:
def __setitem__(self, cls, pointer_type):
cls.pointer_type = pointer_type
def __getitem__(self, cls):
try:
return cls.pointer_type
except AttributeError:
raise KeyError(cls)
def __contains__(self, cls):
return hasattr(cls, 'pointer_type')
_pointer_type_cache = PointerTypeCache() Would having all the info in one place be worth that? Or am I overthinking it, and we should fix the immediate issue and merge this PR as is? |
AFAIU, if we introduce I'm not that familiar with
If we allow to poison regular Python classes with our |
Co-authored-by: neonene <53406459+neonene@users.noreply.github.com>
Just in my gut feeling, but would the following make sense?
|
Yeah, this is like @encukou proposed for PointerTypeCache with addition of mapping interface that would be useful for introspection for example. But please see my reply about incomplete types and non-ctypes types above. |
As far as I can see, only three kinds of keys are acceptable in
Is there a use case for putting something else in
A dict-like object, please. Subclassing There's one more aspect to think about: the registration that comtypes does isn't really the right thing for comtypes to do. comtypes needs to add some functionality to all of its types, so it defines subclasses of the ctypes (meta)classes. But, it also wants to add some functionality to pointers to its types, and pointers are normally created on demand. I can't rule out another use case for registration, but I don't see one. Perhaps we can deprecate |
I'm very new to the internals of ctypes, but I agree with three of the use cases of |
Ah! Looks like you're correct, thanks! I'm not on a Windows box now to check.
So, IUnknown is a ctypes type in spirit, but not in reality :) We could expose Some concerns about having
But, given that it is an internal implementation detail, perhaps that's acceptable. |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@encukou Thanks for review. I have fixed your suggestions. Please take a look. |
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.
I sent a few more suggestions as a PR: sergey-miryanov#1
- cache old-style incomplete types, with a "fallback cache" used when
__pointer_type__
is unavailable - allow setting
__pointer_type__
to arbitrary values (_ctypes
doesn't care)
Co-authored-by: Petr Viktorin <encukou@gmail.com>
For me, |
Looks like that's a 3.13 regression. I hope to fix it later today. |
As I wrote yesterday this should fix it sergey-miryanov@aa44997 |
PyType_Modified works now, but setattr is safer; see #133292 (separate PR since it should be backported) |
This is a bit straightfoward.
I keep the global cache for manually registered pairs of pointers and classes, particularly for comtypes (gh-124520, PyCSimpleTypeAsMetaclassTest).