gh-143732: allow dict subclasses to be specialized #148128
gh-143732: allow dict subclasses to be specialized #148128kumaraditya303 wants to merge 18 commits intopython:mainfrom
Conversation
kumaraditya303
requested review from
Fidget-Spinner,
markshannon,
methane,
savannahostrowski and
tomasr8
as code owners
|
@markshannon Can you take a look at this? I'd like get this in time for the beta release. |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
Looks good overall.
The old uops need to be removed and some assertions may no longer be valid.
Also, take care with recorded types. Only strengthen type information on proven or guards, not recorded information.
| PyObject *dict = PyStackRef_AsPyObjectBorrow(dict_st); | ||
|
|
||
| assert(PyAnyDict_CheckExact(dict)); | ||
| assert(PyAnyDict_Check(dict)); |
There was a problem hiding this comment.
Is this necessarily true?
It seems unlikely, not impossible, that for some object Py_TYPE(dict)>tp_as_mapping->mp_ass_subscript == _PyDict_StoreSubscript, but !PyDict_Check(dict)
There was a problem hiding this comment.
I don't think it is possible, that can only happen if someone intentionally sets the slot which would crash at runtime anyways.
There was a problem hiding this comment.
It would only crash if the object used a different layout.
I could imagine Cython, or pynanobind implementing their own dict for some reason and reusing the existing code.
|
GitHub doen't put comments in order, it seems. The "above" in the comments refers to earlier lines, not earlier comments. |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
I still think the assert(PyDict_Check(dict)); asserts should be removed.
Also you need watchers for all mortal types, not just mutable ones.
|
When you're done making the requested changes, leave the comment: |
I removed the assertions now and added watchers for all cases. I have made the requested changes; please review again |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
Documentation build overview
12 files changed ·
|
2 participants
This is revival of #132383 which allowed specialization for dict subclasses which do not override
__getitem__. This PR also allows specialization for dict subclasses which do not override__setitem__.This is primarily targeted for
defaultdictwhich is the most performance critical subclass of dict.