bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

shihai1991 · 2020-02-22T14:31:53Z

https://bugs.python.org/issue1635741

codecov · 2020-02-22T15:02:56Z

Codecov Report

Merging #18608 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18608      +/-   ##
==========================================
+ Coverage   82.11%   82.13%   +0.01%     
==========================================
  Files        1956     1955       -1     
  Lines      589425   584605    -4820     
  Branches    44458    44484      +26     
==========================================
- Hits       484018   480164    -3854     
+ Misses      95752    94793     -959     
+ Partials     9655     9648       -7

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Modules/_decimal/libmpdec/umodarith.h	`80.76% <0.00%> (-19.24%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
... and 352 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c56f8f...5364b74. Read the comment docs.

shihai1991 · 2020-02-22T15:42:35Z

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

encukou · 2020-02-25T15:43:37Z

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)?

shihai1991 · 2020-02-25T16:41:57Z

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 ;)

…python into bpo_1635741_audioop

shihai1991 · 2020-02-26T09:03:05Z

petr, it's failed again, so I try to find a win env to debug~

vstinner · 2020-02-29T23:01:50Z

Modules/audioop.c

+audioop_traverse(PyObject *m, visitproc visit, void *arg)
+{
+    audioop_state *state = (audioop_state *)PyModule_GetState(m);
+    if (state) {


Is it really possible that state can be NULL when this function is called?

vstinner · 2020-02-29T23:01:54Z

Modules/audioop.c

+audioop_clear(PyObject *m)
+{
+    audioop_state *state = (audioop_state *)PyModule_GetState(m);
+    if (state) {


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.

I would prefer to see this question answered before approving the PR.

See the docs.

Is it worth us to change this logic as victor said?

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.

Modules/audioop.c

Co-Authored-By: Victor Stinner <[email protected]>

shihai1991 · 2020-03-01T04:33:35Z

Oh, gate have passed, thanks, victor.

Port audioop extension module to multiphase initialization(PEP 489)

42b2a61

the-knights-who-say-ni added the CLA signed label Feb 22, 2020

bedevere-bot added the awaiting review label Feb 22, 2020

📜🤖 Added by blurb_it.

981bac9

shihai1991 requested review from encukou, vstinner and ncoghlan February 22, 2020 14:36

shihai1991 removed request for vstinner, encukou and ncoghlan February 23, 2020 08:14

shihai1991 changed the title ~~bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)~~ [WIP] bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) Feb 23, 2020

shihai1991 mentioned this pull request Feb 25, 2020

bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489) #18358

Merged

shihai1991 added 2 commits February 26, 2020 01:04

port _audioopstate() macro to inline function

b776cb1

Merge branch 'bpo_1635741_audioop' of https://github.com/shihai1991/c…

8434b61

…python into bpo_1635741_audioop

shihai1991 requested a review from encukou February 25, 2020 17:14

vstinner reviewed Feb 29, 2020

View reviewed changes

Modules/audioop.c Outdated Show resolved Hide resolved

Update Modules/audioop.c

1a1b56e

Co-Authored-By: Victor Stinner <[email protected]>

shihai1991 changed the title ~~[WIP] bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)~~ bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) Mar 1, 2020

shihai1991 added 2 commits March 1, 2020 16:06

using the var name module to replace m

cfa67a7

using the var name module to replace m in get_audioop_state()

5364b74

vstinner merged commit 41fbf86 into python:master Mar 11, 2020

bedevere-bot removed the awaiting review label Mar 11, 2020

kylotan mannequin mentioned this pull request Sep 19, 2022

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

shihai1991 commented Feb 22, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 22, 2020 •

edited

Loading

shihai1991 commented Feb 22, 2020

encukou commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

shihai1991 commented Feb 26, 2020

vstinner Feb 29, 2020

vstinner Feb 29, 2020

vstinner Mar 1, 2020

encukou Mar 1, 2020 •

edited

Loading

shihai1991 Mar 2, 2020

vstinner Mar 2, 2020

shihai1991 commented Mar 1, 2020

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

Conversation

shihai1991 commented Feb 22, 2020 • edited by bedevere-bot Loading

codecov bot commented Feb 22, 2020 • edited Loading

Codecov Report

shihai1991 commented Feb 22, 2020

encukou commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

shihai1991 commented Feb 26, 2020

vstinner Feb 29, 2020

Choose a reason for hiding this comment

vstinner Feb 29, 2020

Choose a reason for hiding this comment

vstinner Mar 1, 2020

Choose a reason for hiding this comment

encukou Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

shihai1991 Mar 2, 2020

Choose a reason for hiding this comment

vstinner Mar 2, 2020

Choose a reason for hiding this comment

shihai1991 commented Mar 1, 2020

shihai1991 commented Feb 22, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 22, 2020 •

edited

Loading

encukou Mar 1, 2020 •

edited

Loading