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, visitprocvisit, 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 <vstinner@python.org>

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, ncoghlan and vstinner February 22, 2020 14:36

shihai1991 removed request for encukou, ncoghlan and vstinner 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' ofhttps://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 resolvedHide resolved

Update Modules/audioop.c
1a1b56e
Co-Authored-By: Victor Stinner <vstinner@python.org>

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:masterMar 11, 2020

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

kylotanmannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed

Uh oh!

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

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

Uh oh!

Conversation

shihai1991 commented Feb 22, 2020• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovbot commented Feb 22, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shihai1991 commented Feb 22, 2020

Uh oh!

encukou commented Feb 25, 2020

Uh oh!

shihai1991 commented Feb 25, 2020

Uh oh!

shihai1991 commented Feb 26, 2020

Uh oh!

vstinnerFeb 29, 2020

Choose a reason for hiding this comment

Uh oh!

vstinnerFeb 29, 2020

Choose a reason for hiding this comment

Uh oh!

vstinnerMar 1, 2020

Choose a reason for hiding this comment

Uh oh!

encukouMar 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihai1991Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

vstinnerMar 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shihai1991 commented Mar 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

codecovbot commented Feb 22, 2020•
edited
Loading

encukouMar 1, 2020•
edited
Loading