-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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-1635741: Port _collections module to multiphase initialization. #19074
bpo-1635741: Port _collections module to multiphase initialization. #19074
Conversation
For the record, I will work on a new helper function after this PR is landed if this work is worth to do. |
@shihai1991 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.
LGTM apart "m" variable name which looks too short to me ;-)
if (PyType_Ready(type) < 0) { | ||
return -1; | ||
} | ||
const char *name = _PyType_Name(type); |
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.
FYI I checked manually that the actual type name and _PyType_Name() gives the same name than before this change.
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 did same thing also :) Thank you for the re-checking!
Modules/_collectionsmodule.c
Outdated
} | ||
const char *name = _PyType_Name(type); | ||
Py_INCREF(type); | ||
if (PyModule_AddObject(m, name, (PyObject *)type) < 0) { |
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.
Checking for PyModule_AddObject() error is a good thing ;-)
Modules/_collectionsmodule.c
Outdated
_COLLECTIONS__COUNT_ELEMENTS_METHODDEF | ||
{NULL, NULL} /* sentinel */ | ||
}; | ||
|
||
static int | ||
collections_exec(PyObject *m) { |
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.
nitpick: would you mind to rename it to "mod" or "module"?
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.
module might be great! Thanks for the suggestion
oh, on~ my network crashed at this point :( |
|
||
defdict_type.tp_base = &PyDict_Type; | ||
|
||
for (int i = 0; typelist[i] != NULL; i++) { |
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.
nice improvment.
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 like the improvment of typelist.
LGTM :)
Thanks for the reviews :) |
https://bugs.python.org/issue1635741