-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-43977: Make sure that tp_flags for pattern matching are inherited correctly. #25813
bpo-43977: Make sure that tp_flags for pattern matching are inherited correctly. #25813
Conversation
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.
Thanks!
It looks like there might a typo (base
instead of type
) in typeobject.c
:
Objects/typeobject.c
Outdated
if ((base->tp_flags & COLLECTION_FLAGS) == 0) { | ||
return; | ||
} | ||
base->tp_flags |= base->tp_flags & COLLECTION_FLAGS; |
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 think you mean type->tp_flags
on the LHS of the last assignment, right?
Also, the base->tp_flags & COLLECTION_FLAGS
check seems unnecessary.
if ((base->tp_flags & COLLECTION_FLAGS) == 0) { | |
return; | |
} | |
base->tp_flags |= base->tp_flags & COLLECTION_FLAGS; | |
type->tp_flags |= base->tp_flags & COLLECTION_FLAGS; |
Modules/_abc.c
Outdated
/* Set Py_TPFLAGS_SEQUENCE or Py_TPFLAGS_MAPPING flag */ | ||
if (PyType_Check(subclass) && PyType_Check(self) && | ||
!PyType_HasFeature((PyTypeObject *)subclass, Py_TPFLAGS_IMMUTABLETYPE)) | ||
{ | ||
((PyTypeObject *)subclass)->tp_flags |= (((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS); | ||
if (((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS) { | ||
((PyTypeObject *)subclass)->tp_flags &= ~COLLECTION_FLAGS; | ||
((PyTypeObject *)subclass)->tp_flags |= (((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS); | ||
} | ||
} |
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.
This might read better if the nested "if" conditions are merged. Also (looking at this again), I don't think the first PyType_Check
call is needed.
So maybe something like:
/* Set Py_TPFLAGS_SEQUENCE or Py_TPFLAGS_MAPPING flag */
if (PyType_Check(self) &&
PyType_HasFeature((PyTypeObject *)self, COLLECTION_FLAGS) &&
!PyType_HasFeature((PyTypeObject *)subclass, Py_TPFLAGS_IMMUTABLETYPE))
{
((PyTypeObject *)subclass)->tp_flags &= ~COLLECTION_FLAGS;
((PyTypeObject *)subclass)->tp_flags |= (((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS);
}
(GitHub won't let me suggest changing lines that aren't part of the diff.)
When you're done making the requested changes, leave the comment: |
…only. Tidy up code in abc.register
@brandtbucher I've made your requested changes. |
Something we have yet to consider (can wait until after the freeze though): how do we want to handle classes that are registered as mappings or sequences after being subclassed? It's a strange case... do we go through and recursively update the flags of all children, or just say "that won't work for your previously-defined children" (perhaps with a warning at register time)? |
Makes sure that
Py_TPFLAGS_SEQUENCE
andPy_TPFLAGS_MAPPING
are mutually exclusive.https://bugs.python.org/issue43977