-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-101819: Prepare to modernize the _io extension #104178
Conversation
This PR is based on @erlend-aasland's PR #101948. Changes:
I copied the clinic_state() macros.
I was not sure that it works on static types... but it seems like it does :-) I didn't get any crash. |
@corona10 @erlend-aasland: Would you mind to review it? |
sure |
Oh, the Windows change didn't go well:
|
The failed assertion is:
in _io__WindowsConsoleIO___init___impl(). The assertion fails because PyType_GetModuleByDef() returns NULL. |
On static types, PyType_GetModuleByDef() always returns NULL, since the function reads
|
This comment was marked as outdated.
This comment was marked as outdated.
* Add references to static types to _PyIO_State: * PyBufferedIOBase_Type * PyBytesIOBuffer_Type * PyIncrementalNewlineDecoder_Type * PyRawIOBase_Type * PyTextIOBase_Type * Add the defining class to methods: * _io.BytesIO.getbuffer() * _io.FileIO.close() * Add get_io_state_by_cls() function. * Add state parameter to _textiowrapper_decode() * _io_TextIOWrapper___init__() now sets self->state before calling _textiowrapper_set_decoder(). Co-Authored-by: Erlend E. Aasland <[email protected]>
Using APIs to get the module state from a type works on heap types like _io.FileIO, but failed on _io.WindowsConsoleIO which remains a static type with my PR. I fixed my PR by reverting changes related to WindowsConsoleIO: this type should be modified in a following PR. |
I reverted changes related to PyWindowsConsoleIO_Type and _io._WindowsConsoleIO.close(). |
I prefer doing Windows only changes in separate PRs. Everytime I touch Windows stuff, something weird happen :) |
Aha, now the CI pass. I would feel more comfortable with a review. @erlend-aasland: you can also review if you want, to check if i picked the right parts or tour code 😁 |
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, let's wait refleak bot.
I missed something: Erlend already converted multiple _io types to heap types: see commit c00faf7. Types used in this PR are heap types, that's why it just works! |
Thanks for running these jobs. I ran refleaks locally on test_io, but i don't trust myself 🤣 So having reliable buildbot checks sound safer. I will merge my PR once the Refleaks jobs pass, unless someome wants me to wait for their second review. |
No refleaks; good bot 🤖 ✅ |
* Add references to static types to _PyIO_State: * PyBufferedIOBase_Type * PyBytesIOBuffer_Type * PyIncrementalNewlineDecoder_Type * PyRawIOBase_Type * PyTextIOBase_Type * Add the defining class to methods: * _io.BytesIO.getbuffer() * _io.FileIO.close() * Add get_io_state_by_cls() function. * Add state parameter to _textiowrapper_decode() * _io_TextIOWrapper___init__() now sets self->state before calling _textiowrapper_set_decoder(). Co-authored-by: Erlend E. Aasland <[email protected]>
Add references to static types to _PyIO_State:
Add the defining class to methods:
Add get_io_state_by_cls() function.
Add state parameter to _textiowrapper_decode()
io_TextIOWrapper___init_() now sets self->state before calling _textiowrapper_set_decoder().
_io
extension module #101819