bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) by shihai1991 · Pull Request #18608 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits7 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Looks there have an potential bug, because running test_audioop test case, the error name not belong to audioop: binascii.Error: Input sample should be longer
bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) [WIP] bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)
I don't have a Windows machine to test this on.
But, I see that the struct name and the macro to get it are both called _audioopstate! I missed that in my earlier review :(
Could you first turn the macro into an inline function, and rename the type (to something like audioop_state)?
I don't have a Windows machine to test this on.
But, I see that the struct name and the macro to get it are both called_audioopstate! I missed that in my earlier review :(Could you first turn the macro into an inline function, and rename the type (to something like
audioop_state)?
of course, let me try it now ;)
petr, it's failed again, so I try to find a win env to debug~
| audioop_traverse(PyObject *m, visitproc visit, void *arg) |
|---|
| { |
| audioop_state *state = (audioop_state *)PyModule_GetState(m); |
| if (state) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really possible that state can be NULL when this function is called?
| audioop_clear(PyObject *m) |
|---|
| { |
| audioop_state *state = (audioop_state *)PyModule_GetState(m); |
| if (state) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really possible that state can be NULL when this function is called?
module_clear() doesn't touch module->md_state value. Only module_dealloc() calls PyMem_FREE(m->md_state). But audioop_clear() must not be called on a deallocated module object.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to see this question answered before approving the PR.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth us to change this logic as victor said?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created https://bugs.python.org/issue39824 to propose to change the Python behavior, so modules would not have to handle md_state=NULL anymore.
Co-Authored-By: Victor Stinner vstinner@python.org
[WIP] bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)
Oh, gate have passed, thanks, victor.
