bpo-38303: Make audioop extension module PEP-384 compatible #16497

tkieft · 2019-09-30T16:43:55Z

https://bugs.python.org/issue38303

Automerge-Triggered-By: @encukou

the-knights-who-say-ni · 2019-09-30T16:43:58Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@tkieft

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

eduardo-elizondo · 2019-09-30T17:01:30Z

Modules/audioop.c

+        PyModule_AddObject(m, "error", AudioopError);
+        _audioopstate(m)->AudioopError = AudioopError;
+    }
+    PyState_AddModule(m, &audioopmodule);


Let's remove this line here. This is being discussed in PR16101 and very likely the documentation will change to indicate that it's not needed

Modules/audioop.c

eduardo-elizondo · 2019-09-30T17:06:47Z

Modules/audioop.c

-    if (AudioopError != NULL)
-         PyDict_SetItemString(d,"error",AudioopError);
+    PyObject *AudioopError = PyErr_NewException("audioop.error", NULL, NULL);
+    if (AudioopError != NULL) {


Let's follow what most extensions do and return NULL if this fails. An example of this is: _localemodule

eduardo-elizondo · 2019-09-30T17:13:29Z

Take a look at the following URL and add a NEWS file about your change: https://devguide.python.org/committing/#what-s-new-and-news-entries

Also, change your title to: "Update audioop.c to PEP-384" Like: PR15975

eduardo-elizondo

A couple of changes.

tkieft · 2019-09-30T20:37:31Z

Thanks for the review @eduardo-elizondo !

tkieft · 2019-09-30T20:54:24Z

LOL reading my title again, I was looking at the zlib diff and it was late on a friday smh :|

eduardo-elizondo

LGTM!

eduardo-elizondo · 2019-10-01T14:55:56Z

cc: @tiran, @encukou could you take a quick look?

encukou · 2019-10-08T10:50:35Z

Misc/NEWS.d/next/C API/2019-09-30-16-53-30.bpo-38303.YoIs0M.rst

@@ -0,0 +1 @@
+Update audioop extension module to conform to PEP-384


Suggested change

Update audioop extension module to conform to PEP-384

Update audioop extension module to use the stable ABI (PEP-384)

This is a bit of a pet peeve of mine: PEP numbers are only understandable to people who work with that particular PEP. The What's New document deserves better :)

encukou · 2019-10-08T10:58:40Z

Modules/audioop.c

+} _audioopstate;
+
+#define _audioopstate(o) ((_audioopstate *)PyModule_GetState(o))
+#define _audioopstate_global _audioopstate(PyState_FindModule(&audioopmodule))


_audioopstate_global should be unnecessary. The module is readily available at all call sites (except for two helper functions whose callers have the module ready).
Using _audioopstate(module) instead should be both faster and more future-proof (see PEP 489, multi-phase init).

encukou · 2019-10-08T11:01:03Z

Modules/audioop.c

+static int
+audioop_traverse(PyObject *m, visitproc visit, void *arg) {
+    Py_VISIT(_audioopstate(m)->AudioopError);
+    return 0;
+}
+static int
+audioop_clear(PyObject *m) {
+    Py_CLEAR(_audioopstate(m)->AudioopError);
+    return 0;
+}
+static void
+audioop_free(void *m) {
+    audioop_clear((PyObject *)m);
+}


For all three functions, the module state may be NULL. Please check the possibility.
(AudioopError may be NULL as well, but the Py_VISIT/Py_CLEAR macros deal with that.)

encukou · 2019-10-08T11:03:11Z

Modules/audioop.c

+    PyObject *m;
+    m = PyState_FindModule(&audioopmodule);
+    if (m != NULL) {
+        Py_INCREF(m);
+        return m;
+    }


This is unnecessary; the import mechanism should ensure only one copy of the extension module is imported.

encukou · 2019-10-08T11:11:33Z

cc @Dormouse759, this might be interesting to you.

tkieft · 2019-10-10T19:44:47Z

Thanks for the detailed review @encukou - made those changes!

tkieft · 2019-10-21T09:17:09Z

@encukou - Mind taking another look?

encukou

@tkieft, I hope you don't mind your name in the ACKs. If so, this is good to merge.

tkieft · 2019-10-22T12:17:19Z

Don't mind at all! Thanks!

miss-islington · 2019-10-22T12:24:50Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

encukou · 2019-10-22T12:41:28Z

@zooba What's the best way to get Azure Pipelines to run on this PR?
I see some mailing list discussion on this from May; is "close & re-open" is still the way to go? Perhaps long-term enough to document it in the devguide?

miss-islington · 2019-10-22T12:42:05Z

@tkieft: Status check is done, and it's a success ✅ .

miss-islington · 2019-10-22T12:42:06Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

miss-islington · 2019-10-22T12:44:46Z

@tkieft: Status check is done, and it's a success ✅ .

miss-islington · 2019-10-22T12:44:48Z

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

miss-islington · 2019-10-22T13:07:02Z

@tkieft: Status check is done, and it's a success ✅ .

@encukou

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

@encukou

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

the-knights-who-say-ni added the CLA not signed label Sep 30, 2019

bedevere-bot added the awaiting review label Sep 30, 2019

eduardo-elizondo reviewed Sep 30, 2019

View reviewed changes

Modules/audioop.c Show resolved Hide resolved

eduardo-elizondo reviewed Sep 30, 2019

View reviewed changes

eduardo-elizondo suggested changes Sep 30, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 30, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 30, 2019

tkieft changed the title ~~bpo-38303: Make audioop extension module zlib-compatible~~ bpo-38303: Make audioop extension module PEP-384 compatible Sep 30, 2019

eduardo-elizondo approved these changes Oct 1, 2019

View reviewed changes

encukou reviewed Oct 8, 2019

View reviewed changes

Update audioop.c to use the stable ABI (PEP-384)

44c5624

Add author to ACKS

a8e9c85

encukou approved these changes Oct 22, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 22, 2019

encukou added the 🤖 automerge label Oct 22, 2019

miss-islington merged commit f548a3e into python:master Oct 22, 2019

bedevere-bot removed the awaiting merge label Oct 22, 2019

tkieft deleted the pep384/audioop branch October 22, 2019 13:10

encukou mentioned this pull request Oct 22, 2019

bpo-37645: add new function _PyObject_FunctionStr() #14890

Merged

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38303: Make audioop extension module PEP-384 compatible (pythonGH…

eef0831

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38303: Make audioop extension module PEP-384 compatible (pythonGH…

c084fb2

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

		@@ -0,0 +1 @@
		Update audioop extension module to conform to PEP-384

	Update audioop extension module to conform to PEP-384
	Update audioop extension module to use the stable ABI (PEP-384)

Uh oh!

bpo-38303: Make audioop extension module PEP-384 compatible #16497

bpo-38303: Make audioop extension module PEP-384 compatible #16497

Uh oh!

Conversation

tkieft commented Sep 30, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 30, 2019

CLA Missing

Uh oh!

eduardo-elizondo Sep 30, 2019 • 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!

Uh oh!

eduardo-elizondo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo commented Sep 30, 2019

Uh oh!

eduardo-elizondo left a comment

Choose a reason for hiding this comment

Uh oh!

tkieft commented Sep 30, 2019

Uh oh!

tkieft commented Sep 30, 2019

Uh oh!

eduardo-elizondo left a comment

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo commented Oct 1, 2019

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou commented Oct 8, 2019

Uh oh!

tkieft commented Oct 10, 2019

Uh oh!

tkieft commented Oct 21, 2019

Uh oh!

encukou left a comment • 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!

tkieft commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

encukou commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

Uh oh!

tkieft commented Sep 30, 2019 •

edited by miss-islington

Loading

eduardo-elizondo Sep 30, 2019 •

edited

Loading

encukou left a comment •

edited

Loading