-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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 overlapped to multi-phase #22051
Conversation
|
||
#define WINAPI_CONSTANT(fmt, con) \ |
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 that this can be converted into PyModule_AddObject
instead of using PyModule_GetDict
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.
@corona10 You're right. Is that preferable to PyModule_GetDict? If so why?
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.
https://docs.python.org/3/c-api/module.html?highlight=pymodule_getdict#c.PyModule_GetDict
It is recommended extensions use other PyModule_*() and PyObject_*() functions rather than directly manipulate a module’s __dict__.
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.
@corona10 I'm trying that now. So far it is not looking good - somehow the values are all wrong. For example when I switch to PyModule_AddObject
, overlapped.__dict__["ERROR_IO_PENDING"]
is 6160 instead of 997. I can't figure out why..
Edit - I figured it out. I was doing a decref on the object I passed into PyModule_AddObject, but it turns out this function steals the reference so I shouldn't do that.
@vstinner @shihai1991 @corona10 please review |
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 a typo in the NEWS entry. Please fix the NEWS entry, so I can merge your PR.
@@ -0,0 +1,2 @@ | |||
Port the :mod:`overlapped` extension module to multi-phase initialization |
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.
The module is named "_overlapped" with an underscore.
Port the :mod:`overlapped` extension module to multi-phase initialization | |
Port the :mod:`_overlapped` extension module to multi-phase initialization |
Modules/overlapped.c
Outdated
return -1; | ||
} | ||
|
||
Py_DECREF(socket_module); | ||
|
||
if (initialize_function_pointers() < 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.
While you modify the if, please add braces for consistency with the function (PEP 7).
* origin/master: (1373 commits) bpo-1635741: Port mashal module to multi-phase init (python#22149) bpo-1635741: Port _string module to multi-phase init (pythonGH-22148) bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134) bpo-1635741: Port the termios to multi-phase init (PEP 489) (pythonGH-22139) bpo-41732: add iterator to memoryview (pythonGH-22119) bpo-40744: Drop support for SQLite pre 3.7.3 (pythonGH-20909) bpo-41316: Make tarfile follow specs for FNAME (pythonGH-21511) bpo-41720: Add "return NotImplemented" in turtle.Vec2D.__rmul__(). (pythonGH-22092) bpo-1635741 port _curses_panel to multi-phase init (PEP 489) (pythonGH-21986) bpo-1635741: Port _overlapped module to multi-phase init (pythonGH-22051) bpo-1635741: Port _opcode module to multi-phase init (PEP 489) (pythonGH-22050) bpo-1635741 port zlib module to multi-phase init (pythonGH-21995) [doc] Add link to Generic in typing (pythonGH-22125) bpo-41513: Expand comments and add references for a better understanding (pythonGH-22123) bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-21818) closes bpo-41723: Fix an error in the py_compile documentation. (pythonGH-22110) [doc] Fix padding in some typing definitions (pythonGH-22114) Fix documented Python version for venv --upgrade-deps (pythonGH-22113) bpo-40318: Migrate to SQLite3 trace v2 API (pythonGH-19581) bpo-41687: Fix sendfile implementation to work with Solaris (python#22040) ...
https://bugs.python.org/issue1635741