gh-112050: Convert most methods on collections.deque to use Argument Clinic #113963

mpage · 2024-01-11T22:53:52Z

This PR converts most methods on collections.deque to use Argument Clinic as a first step towards making collections.deque thread-safe in --disable-gil builds. A subsequent PR will take advantage of this by using Argument Clinic's @critical_section directive to wrap these methods in a critical section when the GIL is disabled.

I've tried to keep the docstring output for help(deque) identical, except for the cases where the docstring contained information that was already present in the help text. The diff between the original and new output of help(deque) is here. Happy to keep the docstrings identical if the changes are undesirable.

Issue: Make collections.deque thread-safe in --disable-gil builds #112050

Convert most methods on collections.deque to use clinic. This will allow us to use clinic's `@critical_section` directive when making deques thread-safe for `--gil-disabled`, simplifying the implementation.

We pop the leftmost, not the rightmost.

The original version is ``` deque([iterable[, maxlen]]) --> deque object ``` The latter part seems redundant so has been omitted.

Modules/_collectionsmodule.c

aisk · 2024-01-14T16:22:30Z

In the Argument Clinic declarations in this PR, the parameter which is an element of the deque container have different names like item, or v, or value.

Argument Clinic will also generate the function signature with this information. One can access it with deque().count.__text_signature__ or using the inspect module. Some linter tools or type checkers will use this information too.

Although most methods of deque's parameters are position-only and don't affect users' code, using a fixed name to making it consistent is always good if there is no historical problems.

I'm not sure if it's necessary. Maybe @erlend-aasland can take a look.

mpage · 2024-01-17T03:48:08Z

Can you update the PR to use the self converter for all the deque params.

@erlend-aasland - I think I'm already doing this. I'm subclassing self_converter here and using that for all instances of the deque param (example). Is that not the case?

erlend-aasland · 2024-01-17T07:54:57Z

I think I'm already doing this. I'm subclassing self_converter here and using that for all instances of the deque param (example). Is that not the case?

Ah, you are correct; that does work, though a little bit untraditional 😉 Nice hack!

erlend-aasland

erlend-aasland left a comment

LGTM in general, but I'd like the following changes in order to reduce the diff:

Use the Argument as keyword so we keep the C function names unchanged: _collections.deque.index as deque_index, etc. This greatly reduces the diff/churn. See proposed patch below.

Patch against PR

diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c
index fbd45ccc24..76fc66d1cc 100644
--- a/Modules/_collectionsmodule.c
+++ b/Modules/_collectionsmodule.c
@@ -229,7 +229,7 @@ deque_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 }
 
 /*[clinic input]
-_collections.deque.pop
+_collections.deque.pop as deque_pop
 
     deque: dequeobject
 
@@ -237,8 +237,8 @@ Remove and return the rightmost element.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_pop_impl(dequeobject *deque)
-/*[clinic end generated code: output=2d4ef1dcd5113ae6 input=b4873fc20283d8d6]*/
+deque_pop_impl(dequeobject *deque)
+/*[clinic end generated code: output=2e5f7890c4251f07 input=eb6e6d020f877dec]*/
 {
     PyObject *item;
     block *prevblock;
@@ -273,7 +273,7 @@ _collections_deque_pop_impl(dequeobject *deque)
 }
 
 /*[clinic input]
-_collections.deque.popleft
+_collections.deque.popleft as deque_popleft
 
      deque: dequeobject
 
@@ -281,8 +281,8 @@ Remove and return the leftmost element.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_popleft_impl(dequeobject *deque)
-/*[clinic end generated code: output=8cd77178b5116aba input=0ca92ec89734848a]*/
+deque_popleft_impl(dequeobject *deque)
+/*[clinic end generated code: output=62b154897097ff68 input=acb41b9af50a9d9b]*/
 {
     PyObject *item;
     block *prevblock;
@@ -349,7 +349,7 @@ deque_append_internal(dequeobject *deque, PyObject *item, Py_ssize_t maxlen)
     deque->rightindex++;
     deque->rightblock->data[deque->rightindex] = item;
     if (NEEDS_TRIM(deque, maxlen)) {
-        PyObject *olditem = _collections_deque_popleft_impl(deque);
+        PyObject *olditem = deque_popleft_impl(deque);
         Py_DECREF(olditem);
     } else {
         deque->state++;
@@ -358,7 +358,7 @@ deque_append_internal(dequeobject *deque, PyObject *item, Py_ssize_t maxlen)
 }
 
 /*[clinic input]
-_collections.deque.append
+_collections.deque.append as deque_append
 
     deque: dequeobject
     item: object
@@ -368,8 +368,8 @@ Add an element to the right side of the deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_append(dequeobject *deque, PyObject *item)
-/*[clinic end generated code: output=fc44cc7b9dcb0180 input=803e0d976a2e2620]*/
+deque_append(dequeobject *deque, PyObject *item)
+/*[clinic end generated code: output=507b13efc4853ecc input=f112b83c380528e3]*/
 {
     if (deque_append_internal(deque, Py_NewRef(item), deque->maxlen) < 0)
         return NULL;
@@ -394,7 +394,7 @@ deque_appendleft_internal(dequeobject *deque, PyObject *item, Py_ssize_t maxlen)
     deque->leftindex--;
     deque->leftblock->data[deque->leftindex] = item;
     if (NEEDS_TRIM(deque, deque->maxlen)) {
-        PyObject *olditem = _collections_deque_pop_impl(deque);
+        PyObject *olditem = deque_pop_impl(deque);
         Py_DECREF(olditem);
     } else {
         deque->state++;
@@ -403,7 +403,7 @@ deque_appendleft_internal(dequeobject *deque, PyObject *item, Py_ssize_t maxlen)
 }
 
 /*[clinic input]
-_collections.deque.appendleft
+_collections.deque.appendleft as deque_appendleft
 
     deque: dequeobject
     item: object
@@ -413,8 +413,8 @@ Add an element to the left side of the deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_appendleft(dequeobject *deque, PyObject *item)
-/*[clinic end generated code: output=f1b75022fbccf8bb input=481442915f0f6465]*/
+deque_appendleft(dequeobject *deque, PyObject *item)
+/*[clinic end generated code: output=de0335a64800ffd8 input=bbdaa60a3e956062]*/
 {
     if (deque_appendleft_internal(deque, Py_NewRef(item), deque->maxlen) < 0)
         return NULL;
@@ -452,7 +452,7 @@ consume_iterator(PyObject *it)
 }
 
 /*[clinic input]
-_collections.deque.extend
+_collections.deque.extend as deque_extend
 
     deque: dequeobject
     iterable: object
@@ -462,8 +462,8 @@ Extend the right side of the deque with elements from the iterable
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_extend(dequeobject *deque, PyObject *iterable)
-/*[clinic end generated code: output=a58014bf32cb0b9d input=5a75e68f72ed8f09]*/
+deque_extend(dequeobject *deque, PyObject *iterable)
+/*[clinic end generated code: output=a3a6e74d17063f8d input=47722d73a4551312]*/
 {
     PyObject *it, *item;
     PyObject *(*iternext)(PyObject *);
@@ -475,7 +475,7 @@ _collections_deque_extend(dequeobject *deque, PyObject *iterable)
         PyObject *s = PySequence_List(iterable);
         if (s == NULL)
             return NULL;
-        result = _collections_deque_extend(deque, s);
+        result = deque_extend(deque, s);
         Py_DECREF(s);
         return result;
     }
@@ -507,7 +507,7 @@ _collections_deque_extend(dequeobject *deque, PyObject *iterable)
 }
 
 /*[clinic input]
-_collections.deque.extendleft
+_collections.deque.extendleft as deque_extendleft
 
     deque: dequeobject
     iterable: object
@@ -517,8 +517,8 @@ Extend the left side of the deque with elements from the iterable
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_extendleft(dequeobject *deque, PyObject *iterable)
-/*[clinic end generated code: output=0a0df3269097f284 input=8dae4c4f9d852a4c]*/
+deque_extendleft(dequeobject *deque, PyObject *iterable)
+/*[clinic end generated code: output=2dba946c50498c67 input=159710f92c7a4bcd]*/
 {
     PyObject *it, *item;
     PyObject *(*iternext)(PyObject *);
@@ -530,7 +530,7 @@ _collections_deque_extendleft(dequeobject *deque, PyObject *iterable)
         PyObject *s = PySequence_List(iterable);
         if (s == NULL)
             return NULL;
-        result = _collections_deque_extendleft(deque, s);
+        result = deque_extendleft(deque, s);
         Py_DECREF(s);
         return result;
     }
@@ -566,7 +566,7 @@ deque_inplace_concat(dequeobject *deque, PyObject *other)
 {
     PyObject *result;
 
-    result = _collections_deque_extend(deque, other);
+    result = deque_extend(deque, other);
     if (result == NULL)
         return result;
     Py_INCREF(deque);
@@ -575,7 +575,7 @@ deque_inplace_concat(dequeobject *deque, PyObject *other)
 }
 
 /*[clinic input]
-_collections.deque.copy
+_collections.deque.copy as deque_copy
 
     deque: dequeobject
 
@@ -583,8 +583,8 @@ Return a shallow copy of a deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_copy_impl(dequeobject *deque)
-/*[clinic end generated code: output=af1e3831be813117 input=f575fc72c00333d4]*/
+deque_copy_impl(dequeobject *deque)
+/*[clinic end generated code: output=6409b3d1ad2898b5 input=0e22f138bc1fcbee]*/
 {
     PyObject *result;
     dequeobject *old_deque = (dequeobject *)deque;
@@ -601,9 +601,9 @@ _collections_deque_copy_impl(dequeobject *deque)
         /* Fast path for the deque_repeat() common case where len(deque) == 1 */
         if (Py_SIZE(deque) == 1) {
             PyObject *item = old_deque->leftblock->data[old_deque->leftindex];
-            rv = _collections_deque_append(new_deque, item);
+            rv = deque_append(new_deque, item);
         } else {
-            rv = _collections_deque_extend(new_deque, (PyObject *)deque);
+            rv = deque_extend(new_deque, (PyObject *)deque);
         }
         if (rv != NULL) {
             Py_DECREF(rv);
@@ -629,16 +629,16 @@ _collections_deque_copy_impl(dequeobject *deque)
 }
 
 /*[clinic input]
-_collections.deque.__copy__ = _collections.deque.copy
+_collections.deque.__copy__ as deque___copy__ = _collections.deque.copy
 
 Return a shallow copy of a deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque___copy___impl(dequeobject *deque)
-/*[clinic end generated code: output=c4c31949334138fd input=9d78c00375929799]*/
+deque___copy___impl(dequeobject *deque)
+/*[clinic end generated code: output=7c5821504342bf23 input=fce05df783e7912b]*/
 {
-    return _collections_deque_copy_impl(deque);
+    return deque_copy_impl(deque);
 }
 
 static PyObject *
@@ -658,10 +658,10 @@ deque_concat(dequeobject *deque, PyObject *other)
         return NULL;
     }
 
-    new_deque = _collections_deque_copy_impl(deque);
+    new_deque = deque_copy_impl(deque);
     if (new_deque == NULL)
         return NULL;
-    result = _collections_deque_extend((dequeobject *)new_deque, other);
+    result = deque_extend((dequeobject *)new_deque, other);
     if (result == NULL) {
         Py_DECREF(new_deque);
         return NULL;
@@ -747,7 +747,7 @@ deque_clear(dequeobject *deque)
 
   alternate_method:
     while (Py_SIZE(deque)) {
-        item = _collections_deque_pop_impl(deque);
+        item = deque_pop_impl(deque);
         assert (item != NULL);
         Py_DECREF(item);
     }
@@ -755,7 +755,7 @@ deque_clear(dequeobject *deque)
 }
 
 /*[clinic input]
-_collections.deque.clear
+_collections.deque.clear as deque_clearmethod
 
     deque: dequeobject
 
@@ -763,8 +763,8 @@ Remove all elements from the deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_clear_impl(dequeobject *deque)
-/*[clinic end generated code: output=0f0b9d60188bf83b input=9c003117680a7abf]*/
+deque_clearmethod_impl(dequeobject *deque)
+/*[clinic end generated code: output=79b2513e097615c1 input=20488eb932f89f9e]*/
 {
     deque_clear(deque);
     Py_RETURN_NONE;
@@ -835,7 +835,7 @@ deque_inplace_repeat(dequeobject *deque, Py_ssize_t n)
         n = (deque->maxlen + size - 1) / size;
 
     for (i = 0 ; i < n-1 ; i++) {
-        rv = _collections_deque_extend(deque, seq);
+        rv = deque_extend(deque, seq);
         if (rv == NULL) {
             Py_DECREF(seq);
             return NULL;
@@ -853,7 +853,7 @@ deque_repeat(dequeobject *deque, Py_ssize_t n)
     dequeobject *new_deque;
     PyObject *rv;
 
-    new_deque = (dequeobject *)_collections_deque_copy_impl(deque);
+    new_deque = (dequeobject *)deque_copy_impl(deque);
     if (new_deque == NULL)
         return NULL;
     rv = deque_inplace_repeat(new_deque, n);
@@ -1011,7 +1011,7 @@ _deque_rotate(dequeobject *deque, Py_ssize_t n)
 }
 
 /*[clinic input]
-_collections.deque.rotate
+_collections.deque.rotate as deque_rotate
 
     deque: dequeobject
     n: Py_ssize_t = 1
@@ -1021,8 +1021,8 @@ Rotate the deque n steps to the right.  If n is negative, rotates left.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_rotate_impl(dequeobject *deque, Py_ssize_t n)
-/*[clinic end generated code: output=5a9df290cc0d3adf input=0d7f4900fe866917]*/
+deque_rotate_impl(dequeobject *deque, Py_ssize_t n)
+/*[clinic end generated code: output=96c2402a371eb15d input=d22070f49cc06c76]*/
 {
     if (!_deque_rotate(deque, n))
         Py_RETURN_NONE;
@@ -1030,7 +1030,7 @@ _collections_deque_rotate_impl(dequeobject *deque, Py_ssize_t n)
 }
 
 /*[clinic input]
-_collections.deque.reverse
+_collections.deque.reverse as deque_reverse
 
     deque: dequeobject
 
@@ -1038,8 +1038,8 @@ Reverse *IN PLACE*
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_reverse_impl(dequeobject *deque)
-/*[clinic end generated code: output=8f859d206158686e input=651e0257414fac22]*/
+deque_reverse_impl(dequeobject *deque)
+/*[clinic end generated code: output=bdeebc2cf8c1f064 input=51c1e0593a8da254]*/
 {
     block *leftblock = deque->leftblock;
     block *rightblock = deque->rightblock;
@@ -1077,7 +1077,7 @@ _collections_deque_reverse_impl(dequeobject *deque)
 }
 
 /*[clinic input]
-_collections.deque.count
+_collections.deque.count as deque_count
 
     deque: dequeobject
     v: object
@@ -1087,8 +1087,8 @@ Return number of occurrences of v
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_count(dequeobject *deque, PyObject *v)
-/*[clinic end generated code: output=4fd47b6bf522f071 input=38d3b9f0f9993e26]*/
+deque_count(dequeobject *deque, PyObject *v)
+/*[clinic end generated code: output=7405d289d94d7b9b input=e90e11dac35aeede]*/
 {
     block *b = deque->leftblock;
     Py_ssize_t index = deque->leftindex;
@@ -1163,7 +1163,7 @@ deque_len(dequeobject *deque)
 
 /*[clinic input]
 @text_signature "($self, value, [start, [stop]])"
-_collections.deque.index
+_collections.deque.index as deque_index
 
     deque: dequeobject
     v: object
@@ -1177,9 +1177,9 @@ Raises ValueError if the value is not present.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_index_impl(dequeobject *deque, PyObject *v,
-                              Py_ssize_t start, Py_ssize_t stop)
-/*[clinic end generated code: output=5b2a991d7315b3cf input=b31d3a5c49cb8725]*/
+deque_index_impl(dequeobject *deque, PyObject *v, Py_ssize_t start,
+                 Py_ssize_t stop)
+/*[clinic end generated code: output=df45132753175ef9 input=5075d3940eef65ec]*/
 {
     Py_ssize_t i, n;
     PyObject *item;
@@ -1248,7 +1248,7 @@ _collections_deque_index_impl(dequeobject *deque, PyObject *v,
 */
 
 /*[clinic input]
-_collections.deque.insert
+_collections.deque.insert as deque_insert
 
     deque: dequeobject
     index: Py_ssize_t
@@ -1259,9 +1259,8 @@ Insert value before index
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_insert_impl(dequeobject *deque, Py_ssize_t index,
-                               PyObject *value)
-/*[clinic end generated code: output=f913d56fc97caddf input=0593cc27bffa766a]*/
+deque_insert_impl(dequeobject *deque, Py_ssize_t index, PyObject *value)
+/*[clinic end generated code: output=ef4d2c15d5532b80 input=e3ff41dae77f5b79]*/
 {
     Py_ssize_t n = Py_SIZE(deque);
     PyObject *rv;
@@ -1271,15 +1270,15 @@ _collections_deque_insert_impl(dequeobject *deque, Py_ssize_t index,
         return NULL;
     }
     if (index >= n)
-        return _collections_deque_append(deque, value);
+        return deque_append(deque, value);
     if (index <= -n || index == 0)
-        return _collections_deque_appendleft(deque, value);
+        return deque_appendleft(deque, value);
     if (_deque_rotate(deque, -index))
         return NULL;
     if (index < 0)
-        rv = _collections_deque_append(deque, value);
+        rv = deque_append(deque, value);
     else
-        rv = _collections_deque_appendleft(deque, value);
+        rv = deque_appendleft(deque, value);
     if (rv == NULL)
         return NULL;
     Py_DECREF(rv);
@@ -1344,7 +1343,7 @@ deque_del_item(dequeobject *deque, Py_ssize_t i)
     assert (i >= 0 && i < Py_SIZE(deque));
     if (_deque_rotate(deque, -i))
         return -1;
-    item = _collections_deque_popleft_impl(deque);
+    item = deque_popleft_impl(deque);
     rv = _deque_rotate(deque, i);
     assert (item != NULL);
     Py_DECREF(item);
@@ -1352,7 +1351,7 @@ deque_del_item(dequeobject *deque, Py_ssize_t i)
 }
 
 /*[clinic input]
-_collections.deque.remove
+_collections.deque.remove as deque_remove
 
     deque: dequeobject
     value: object
@@ -1362,8 +1361,8 @@ Remove first occurrence of value.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque_remove(dequeobject *deque, PyObject *value)
-/*[clinic end generated code: output=6e44d24b93f7109e input=d53d4a0b082137f6]*/
+deque_remove(dequeobject *deque, PyObject *value)
+/*[clinic end generated code: output=49e1666d612fe911 input=d972f32d15990880]*/
 {
     PyObject *item;
     block *b = deque->leftblock;
@@ -1485,7 +1484,7 @@ deque_traverse(dequeobject *deque, visitproc visit, void *arg)
 }
 
 /*[clinic input]
-_collections.deque.__reduce__
+_collections.deque.__reduce__ as deque___reduce__
 
     deque: dequeobject
 
@@ -1493,8 +1492,8 @@ Return state information for pickling.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque___reduce___impl(dequeobject *deque)
-/*[clinic end generated code: output=98e9eed251df2133 input=4210e061fc57e988]*/
+deque___reduce___impl(dequeobject *deque)
+/*[clinic end generated code: output=cb85d9e0b7d2c5ad input=991a933a5bc7a526]*/
 {
     PyObject *state, *it;
 
@@ -1630,36 +1629,36 @@ deque_richcompare(PyObject *v, PyObject *w, int op)
 
 /*[clinic input]
 @text_signature "([iterable[, maxlen]])"
-_collections.deque.__init__
+_collections.deque.__init__ as deque___init__
 
     deque: dequeobject
     iterable: object = NULL
-    maxlen: object = NULL
+    maxlen as maxlenobj: object = NULL
 
 A list-like sequence optimized for data accesses near its endpoints.
 [clinic start generated code]*/
 
 static int
-_collections_deque___init___impl(dequeobject *deque, PyObject *iterable,
-                                 PyObject *maxlen)
-/*[clinic end generated code: output=9fbb306da99f6694 input=aa6219250dc91d12]*/
+deque___init___impl(dequeobject *deque, PyObject *iterable,
+                    PyObject *maxlenobj)
+/*[clinic end generated code: output=548e947960679dd9 input=8a87b7bfabea2cdf]*/
 
 {
-    Py_ssize_t maxlenval = -1;
-    if (maxlen != NULL && maxlen != Py_None) {
-        maxlenval = PyLong_AsSsize_t(maxlen);
-        if (maxlenval == -1 && PyErr_Occurred())
+    Py_ssize_t maxlen = -1;
+    if (maxlenobj != NULL && maxlenobj != Py_None) {
+        maxlen = PyLong_AsSsize_t(maxlenobj);
+        if (maxlen == -1 && PyErr_Occurred())
             return -1;
-        if (maxlenval < 0) {
+        if (maxlen < 0) {
             PyErr_SetString(PyExc_ValueError, "maxlen must be non-negative");
             return -1;
         }
     }
-    deque->maxlen = maxlenval;
+    deque->maxlen = maxlen;
     if (Py_SIZE(deque) > 0)
         deque_clear(deque);
     if (iterable != NULL) {
-        PyObject *rv = _collections_deque_extend(deque, iterable);
+        PyObject *rv = deque_extend(deque, iterable);
         if (rv == NULL)
             return -1;
         Py_DECREF(rv);
@@ -1668,7 +1667,7 @@ _collections_deque___init___impl(dequeobject *deque, PyObject *iterable,
 }
 
 /*[clinic input]
-_collections.deque.__sizeof__
+_collections.deque.__sizeof__ as deque___sizeof__
 
     deque: dequeobject
 
@@ -1676,8 +1675,8 @@ Return the size of the deque in memory, in bytes
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque___sizeof___impl(dequeobject *deque)
-/*[clinic end generated code: output=1a66234430a294a3 input=c0c535e64766f446]*/
+deque___sizeof___impl(dequeobject *deque)
+/*[clinic end generated code: output=4d36e9fb4f30bbaf input=b0678756347de87f]*/
 {
     size_t res = _PyObject_SIZE(Py_TYPE(deque));
     size_t blocks;
@@ -1699,7 +1698,7 @@ deque_get_maxlen(dequeobject *deque, void *Py_UNUSED(ignored))
 static PyObject *deque_reviter(dequeobject *deque);
 
 /*[clinic input]
-_collections.deque.__reversed__
+_collections.deque.__reversed__ as deque___reversed__
 
     deque: dequeobject
 
@@ -1707,8 +1706,8 @@ Return a reverse iterator over the deque.
 [clinic start generated code]*/
 
 static PyObject *
-_collections_deque___reversed___impl(dequeobject *deque)
-/*[clinic end generated code: output=c6980fed84a53cc6 input=8b6299d6d60ea01a]*/
+deque___reversed___impl(dequeobject *deque)
+/*[clinic end generated code: output=3e7e7e715883cf2e input=3d494c25a6fe5c7e]*/
 {
     return deque_reviter(deque);
 }
@@ -1724,24 +1723,24 @@ static PyGetSetDef deque_getset[] = {
 static PyObject *deque_iter(dequeobject *deque);
 
 static PyMethodDef deque_methods[] = {
-    _COLLECTIONS_DEQUE_APPEND_METHODDEF
-    _COLLECTIONS_DEQUE_APPENDLEFT_METHODDEF
-    _COLLECTIONS_DEQUE_CLEAR_METHODDEF
-    _COLLECTIONS_DEQUE___COPY___METHODDEF
-    _COLLECTIONS_DEQUE_COPY_METHODDEF
-    _COLLECTIONS_DEQUE_COUNT_METHODDEF
-    _COLLECTIONS_DEQUE_EXTEND_METHODDEF
-    _COLLECTIONS_DEQUE_EXTENDLEFT_METHODDEF
-    _COLLECTIONS_DEQUE_INDEX_METHODDEF
-    _COLLECTIONS_DEQUE_INSERT_METHODDEF
-    _COLLECTIONS_DEQUE_POP_METHODDEF
-    _COLLECTIONS_DEQUE_POPLEFT_METHODDEF
-    _COLLECTIONS_DEQUE___REDUCE___METHODDEF
-    _COLLECTIONS_DEQUE_REMOVE_METHODDEF
-    _COLLECTIONS_DEQUE___REVERSED___METHODDEF
-    _COLLECTIONS_DEQUE_REVERSE_METHODDEF
-    _COLLECTIONS_DEQUE_ROTATE_METHODDEF
-    _COLLECTIONS_DEQUE___SIZEOF___METHODDEF
+    DEQUE_APPEND_METHODDEF
+    DEQUE_APPENDLEFT_METHODDEF
+    DEQUE_CLEARMETHOD_METHODDEF
+    DEQUE___COPY___METHODDEF
+    DEQUE_COPY_METHODDEF
+    DEQUE_COUNT_METHODDEF
+    DEQUE_EXTEND_METHODDEF
+    DEQUE_EXTENDLEFT_METHODDEF
+    DEQUE_INDEX_METHODDEF
+    DEQUE_INSERT_METHODDEF
+    DEQUE_POP_METHODDEF
+    DEQUE_POPLEFT_METHODDEF
+    DEQUE___REDUCE___METHODDEF
+    DEQUE_REMOVE_METHODDEF
+    DEQUE___REVERSED___METHODDEF
+    DEQUE_REVERSE_METHODDEF
+    DEQUE_ROTATE_METHODDEF
+    DEQUE___SIZEOF___METHODDEF
     {"__class_getitem__",       Py_GenericAlias,
         METH_O|METH_CLASS,       PyDoc_STR("See PEP 585")},
     {NULL,              NULL}   /* sentinel */
@@ -1757,13 +1756,13 @@ static PyType_Slot deque_slots[] = {
     {Py_tp_repr, deque_repr},
     {Py_tp_hash, PyObject_HashNotImplemented},
     {Py_tp_getattro, PyObject_GenericGetAttr},
-    {Py_tp_doc, (void *)_collections_deque___init____doc__},
+    {Py_tp_doc, (void *)deque___init____doc__},
     {Py_tp_traverse, deque_traverse},
     {Py_tp_clear, deque_clear},
     {Py_tp_richcompare, deque_richcompare},
     {Py_tp_iter, deque_iter},
     {Py_tp_getset, deque_getset},
-    {Py_tp_init, _collections_deque___init__},
+    {Py_tp_init, deque___init__},
     {Py_tp_alloc, PyType_GenericAlloc},
     {Py_tp_new, deque_new},
     {Py_tp_free, PyObject_GC_Del},
diff --git a/Modules/clinic/_collectionsmodule.c.h b/Modules/clinic/_collectionsmodule.c.h
index 69b87a33e0..c9467c840f 100644
--- a/Modules/clinic/_collectionsmodule.c.h
+++ b/Modules/clinic/_collectionsmodule.c.h
@@ -9,146 +9,146 @@ preserve
 #include "pycore_abstract.h"      // _PyNumber_Index()
 #include "pycore_modsupport.h"    // _PyArg_CheckPositional()
 
-PyDoc_STRVAR(_collections_deque_pop__doc__,
+PyDoc_STRVAR(deque_pop__doc__,
 "pop($self, /)\n"
 "--\n"
 "\n"
 "Remove and return the rightmost element.");
 
-#define _COLLECTIONS_DEQUE_POP_METHODDEF    \
-    {"pop", (PyCFunction)_collections_deque_pop, METH_NOARGS, _collections_deque_pop__doc__},
+#define DEQUE_POP_METHODDEF    \
+    {"pop", (PyCFunction)deque_pop, METH_NOARGS, deque_pop__doc__},
 
 static PyObject *
-_collections_deque_pop_impl(dequeobject *deque);
+deque_pop_impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque_pop(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque_pop(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque_pop_impl(deque);
+    return deque_pop_impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque_popleft__doc__,
+PyDoc_STRVAR(deque_popleft__doc__,
 "popleft($self, /)\n"
 "--\n"
 "\n"
 "Remove and return the leftmost element.");
 
-#define _COLLECTIONS_DEQUE_POPLEFT_METHODDEF    \
-    {"popleft", (PyCFunction)_collections_deque_popleft, METH_NOARGS, _collections_deque_popleft__doc__},
+#define DEQUE_POPLEFT_METHODDEF    \
+    {"popleft", (PyCFunction)deque_popleft, METH_NOARGS, deque_popleft__doc__},
 
 static PyObject *
-_collections_deque_popleft_impl(dequeobject *deque);
+deque_popleft_impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque_popleft(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque_popleft(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque_popleft_impl(deque);
+    return deque_popleft_impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque_append__doc__,
+PyDoc_STRVAR(deque_append__doc__,
 "append($self, item, /)\n"
 "--\n"
 "\n"
 "Add an element to the right side of the deque.");
 
-#define _COLLECTIONS_DEQUE_APPEND_METHODDEF    \
-    {"append", (PyCFunction)_collections_deque_append, METH_O, _collections_deque_append__doc__},
+#define DEQUE_APPEND_METHODDEF    \
+    {"append", (PyCFunction)deque_append, METH_O, deque_append__doc__},
 
-PyDoc_STRVAR(_collections_deque_appendleft__doc__,
+PyDoc_STRVAR(deque_appendleft__doc__,
 "appendleft($self, item, /)\n"
 "--\n"
 "\n"
 "Add an element to the left side of the deque.");
 
-#define _COLLECTIONS_DEQUE_APPENDLEFT_METHODDEF    \
-    {"appendleft", (PyCFunction)_collections_deque_appendleft, METH_O, _collections_deque_appendleft__doc__},
+#define DEQUE_APPENDLEFT_METHODDEF    \
+    {"appendleft", (PyCFunction)deque_appendleft, METH_O, deque_appendleft__doc__},
 
-PyDoc_STRVAR(_collections_deque_extend__doc__,
+PyDoc_STRVAR(deque_extend__doc__,
 "extend($self, iterable, /)\n"
 "--\n"
 "\n"
 "Extend the right side of the deque with elements from the iterable");
 
-#define _COLLECTIONS_DEQUE_EXTEND_METHODDEF    \
-    {"extend", (PyCFunction)_collections_deque_extend, METH_O, _collections_deque_extend__doc__},
+#define DEQUE_EXTEND_METHODDEF    \
+    {"extend", (PyCFunction)deque_extend, METH_O, deque_extend__doc__},
 
-PyDoc_STRVAR(_collections_deque_extendleft__doc__,
+PyDoc_STRVAR(deque_extendleft__doc__,
 "extendleft($self, iterable, /)\n"
 "--\n"
 "\n"
 "Extend the left side of the deque with elements from the iterable");
 
-#define _COLLECTIONS_DEQUE_EXTENDLEFT_METHODDEF    \
-    {"extendleft", (PyCFunction)_collections_deque_extendleft, METH_O, _collections_deque_extendleft__doc__},
+#define DEQUE_EXTENDLEFT_METHODDEF    \
+    {"extendleft", (PyCFunction)deque_extendleft, METH_O, deque_extendleft__doc__},
 
-PyDoc_STRVAR(_collections_deque_copy__doc__,
+PyDoc_STRVAR(deque_copy__doc__,
 "copy($self, /)\n"
 "--\n"
 "\n"
 "Return a shallow copy of a deque.");
 
-#define _COLLECTIONS_DEQUE_COPY_METHODDEF    \
-    {"copy", (PyCFunction)_collections_deque_copy, METH_NOARGS, _collections_deque_copy__doc__},
+#define DEQUE_COPY_METHODDEF    \
+    {"copy", (PyCFunction)deque_copy, METH_NOARGS, deque_copy__doc__},
 
 static PyObject *
-_collections_deque_copy_impl(dequeobject *deque);
+deque_copy_impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque_copy(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque_copy(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque_copy_impl(deque);
+    return deque_copy_impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque___copy____doc__,
+PyDoc_STRVAR(deque___copy____doc__,
 "__copy__($self, /)\n"
 "--\n"
 "\n"
 "Return a shallow copy of a deque.");
 
-#define _COLLECTIONS_DEQUE___COPY___METHODDEF    \
-    {"__copy__", (PyCFunction)_collections_deque___copy__, METH_NOARGS, _collections_deque___copy____doc__},
+#define DEQUE___COPY___METHODDEF    \
+    {"__copy__", (PyCFunction)deque___copy__, METH_NOARGS, deque___copy____doc__},
 
 static PyObject *
-_collections_deque___copy___impl(dequeobject *deque);
+deque___copy___impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque___copy__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque___copy__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque___copy___impl(deque);
+    return deque___copy___impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque_clear__doc__,
+PyDoc_STRVAR(deque_clearmethod__doc__,
 "clear($self, /)\n"
 "--\n"
 "\n"
 "Remove all elements from the deque.");
 
-#define _COLLECTIONS_DEQUE_CLEAR_METHODDEF    \
-    {"clear", (PyCFunction)_collections_deque_clear, METH_NOARGS, _collections_deque_clear__doc__},
+#define DEQUE_CLEARMETHOD_METHODDEF    \
+    {"clear", (PyCFunction)deque_clearmethod, METH_NOARGS, deque_clearmethod__doc__},
 
 static PyObject *
-_collections_deque_clear_impl(dequeobject *deque);
+deque_clearmethod_impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque_clear(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque_clearmethod(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque_clear_impl(deque);
+    return deque_clearmethod_impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque_rotate__doc__,
+PyDoc_STRVAR(deque_rotate__doc__,
 "rotate($self, n=1, /)\n"
 "--\n"
 "\n"
 "Rotate the deque n steps to the right.  If n is negative, rotates left.");
 
-#define _COLLECTIONS_DEQUE_ROTATE_METHODDEF    \
-    {"rotate", _PyCFunction_CAST(_collections_deque_rotate), METH_FASTCALL, _collections_deque_rotate__doc__},
+#define DEQUE_ROTATE_METHODDEF    \
+    {"rotate", _PyCFunction_CAST(deque_rotate), METH_FASTCALL, deque_rotate__doc__},
 
 static PyObject *
-_collections_deque_rotate_impl(dequeobject *deque, Py_ssize_t n);
+deque_rotate_impl(dequeobject *deque, Py_ssize_t n);
 
 static PyObject *
-_collections_deque_rotate(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
+deque_rotate(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
 {
     PyObject *return_value = NULL;
     Py_ssize_t n = 1;
@@ -172,40 +172,40 @@ _collections_deque_rotate(dequeobject *deque, PyObject *const *args, Py_ssize_t
         n = ival;
     }
 skip_optional:
-    return_value = _collections_deque_rotate_impl(deque, n);
+    return_value = deque_rotate_impl(deque, n);
 
 exit:
     return return_value;
 }
 
-PyDoc_STRVAR(_collections_deque_reverse__doc__,
+PyDoc_STRVAR(deque_reverse__doc__,
 "reverse($self, /)\n"
 "--\n"
 "\n"
 "Reverse *IN PLACE*");
 
-#define _COLLECTIONS_DEQUE_REVERSE_METHODDEF    \
-    {"reverse", (PyCFunction)_collections_deque_reverse, METH_NOARGS, _collections_deque_reverse__doc__},
+#define DEQUE_REVERSE_METHODDEF    \
+    {"reverse", (PyCFunction)deque_reverse, METH_NOARGS, deque_reverse__doc__},
 
 static PyObject *
-_collections_deque_reverse_impl(dequeobject *deque);
+deque_reverse_impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque_reverse(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque_reverse(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque_reverse_impl(deque);
+    return deque_reverse_impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque_count__doc__,
+PyDoc_STRVAR(deque_count__doc__,
 "count($self, v, /)\n"
 "--\n"
 "\n"
 "Return number of occurrences of v");
 
-#define _COLLECTIONS_DEQUE_COUNT_METHODDEF    \
-    {"count", (PyCFunction)_collections_deque_count, METH_O, _collections_deque_count__doc__},
+#define DEQUE_COUNT_METHODDEF    \
+    {"count", (PyCFunction)deque_count, METH_O, deque_count__doc__},
 
-PyDoc_STRVAR(_collections_deque_index__doc__,
+PyDoc_STRVAR(deque_index__doc__,
 "index($self, value, [start, [stop]])\n"
 "--\n"
 "\n"
@@ -213,15 +213,15 @@ PyDoc_STRVAR(_collections_deque_index__doc__,
 "\n"
 "Raises ValueError if the value is not present.");
 
-#define _COLLECTIONS_DEQUE_INDEX_METHODDEF    \
-    {"index", _PyCFunction_CAST(_collections_deque_index), METH_FASTCALL, _collections_deque_index__doc__},
+#define DEQUE_INDEX_METHODDEF    \
+    {"index", _PyCFunction_CAST(deque_index), METH_FASTCALL, deque_index__doc__},
 
 static PyObject *
-_collections_deque_index_impl(dequeobject *deque, PyObject *v,
-                              Py_ssize_t start, Py_ssize_t stop);
+deque_index_impl(dequeobject *deque, PyObject *v, Py_ssize_t start,
+                 Py_ssize_t stop);
 
 static PyObject *
-_collections_deque_index(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
+deque_index(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
 {
     PyObject *return_value = NULL;
     PyObject *v;
@@ -245,27 +245,26 @@ _collections_deque_index(dequeobject *deque, PyObject *const *args, Py_ssize_t n
         goto exit;
     }
 skip_optional:
-    return_value = _collections_deque_index_impl(deque, v, start, stop);
+    return_value = deque_index_impl(deque, v, start, stop);
 
 exit:
     return return_value;
 }
 
-PyDoc_STRVAR(_collections_deque_insert__doc__,
+PyDoc_STRVAR(deque_insert__doc__,
 "insert($self, index, value, /)\n"
 "--\n"
 "\n"
 "Insert value before index");
 
-#define _COLLECTIONS_DEQUE_INSERT_METHODDEF    \
-    {"insert", _PyCFunction_CAST(_collections_deque_insert), METH_FASTCALL, _collections_deque_insert__doc__},
+#define DEQUE_INSERT_METHODDEF    \
+    {"insert", _PyCFunction_CAST(deque_insert), METH_FASTCALL, deque_insert__doc__},
 
 static PyObject *
-_collections_deque_insert_impl(dequeobject *deque, Py_ssize_t index,
-                               PyObject *value);
+deque_insert_impl(dequeobject *deque, Py_ssize_t index, PyObject *value);
 
 static PyObject *
-_collections_deque_insert(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
+dequ
8000
e_insert(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
 {
     PyObject *return_value = NULL;
     Py_ssize_t index;
@@ -287,51 +286,51 @@ _collections_deque_insert(dequeobject *deque, PyObject *const *args, Py_ssize_t
         index = ival;
     }
     value = args[1];
-    return_value = _collections_deque_insert_impl(deque, index, value);
+    return_value = deque_insert_impl(deque, index, value);
 
 exit:
     return return_value;
 }
 
-PyDoc_STRVAR(_collections_deque_remove__doc__,
+PyDoc_STRVAR(deque_remove__doc__,
 "remove($self, value, /)\n"
 "--\n"
 "\n"
 "Remove first occurrence of value.");
 
-#define _COLLECTIONS_DEQUE_REMOVE_METHODDEF    \
-    {"remove", (PyCFunction)_collections_deque_remove, METH_O, _collections_deque_remove__doc__},
+#define DEQUE_REMOVE_METHODDEF    \
+    {"remove", (PyCFunction)deque_remove, METH_O, deque_remove__doc__},
 
-PyDoc_STRVAR(_collections_deque___reduce____doc__,
+PyDoc_STRVAR(deque___reduce____doc__,
 "__reduce__($self, /)\n"
 "--\n"
 "\n"
 "Return state information for pickling.");
 
-#define _COLLECTIONS_DEQUE___REDUCE___METHODDEF    \
-    {"__reduce__", (PyCFunction)_collections_deque___reduce__, METH_NOARGS, _collections_deque___reduce____doc__},
+#define DEQUE___REDUCE___METHODDEF    \
+    {"__reduce__", (PyCFunction)deque___reduce__, METH_NOARGS, deque___reduce____doc__},
 
 static PyObject *
-_collections_deque___reduce___impl(dequeobject *deque);
+deque___reduce___impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque___reduce__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque___reduce__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque___reduce___impl(deque);
+    return deque___reduce___impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque___init____doc__,
+PyDoc_STRVAR(deque___init____doc__,
 "deque([iterable[, maxlen]])\n"
 "--\n"
 "\n"
 "A list-like sequence optimized for data accesses near its endpoints.");
 
 static int
-_collections_deque___init___impl(dequeobject *deque, PyObject *iterable,
-                                 PyObject *maxlen);
+deque___init___impl(dequeobject *deque, PyObject *iterable,
+                    PyObject *maxlenobj);
 
 static int
-_collections_deque___init__(PyObject *deque, PyObject *args, PyObject *kwargs)
+deque___init__(PyObject *deque, PyObject *args, PyObject *kwargs)
 {
     int return_value = -1;
     #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)
@@ -364,7 +363,7 @@ _collections_deque___init__(PyObject *deque, PyObject *args, PyObject *kwargs)
     Py_ssize_t nargs = PyTuple_GET_SIZE(args);
     Py_ssize_t noptargs = nargs + (kwargs ? PyDict_GET_SIZE(kwargs) : 0) - 0;
     PyObject *iterable = NULL;
-    PyObject *maxlen = NULL;
+    PyObject *maxlenobj = NULL;
 
     fastargs = _PyArg_UnpackKeywords(_PyTuple_CAST(args)->ob_item, nargs, kwargs, NULL, &_parser, 0, 2, 0, argsbuf);
     if (!fastargs) {
@@ -379,48 +378,48 @@ _collections_deque___init__(PyObject *deque, PyObject *args, PyObject *kwargs)
             goto skip_optional_pos;
         }
     }
-    maxlen = fastargs[1];
+    maxlenobj = fastargs[1];
 skip_optional_pos:
-    return_value = _collections_deque___init___impl((dequeobject *)deque, iterable, maxlen);
+    return_value = deque___init___impl((dequeobject *)deque, iterable, maxlenobj);
 
 exit:
     return return_value;
 }
 
-PyDoc_STRVAR(_collections_deque___sizeof____doc__,
+PyDoc_STRVAR(deque___sizeof____doc__,
 "__sizeof__($self, /)\n"
 "--\n"
 "\n"
 "Return the size of the deque in memory, in bytes");
 
-#define _COLLECTIONS_DEQUE___SIZEOF___METHODDEF    \
-    {"__sizeof__", (PyCFunction)_collections_deque___sizeof__, METH_NOARGS, _collections_deque___sizeof____doc__},
+#define DEQUE___SIZEOF___METHODDEF    \
+    {"__sizeof__", (PyCFunction)deque___sizeof__, METH_NOARGS, deque___sizeof____doc__},
 
 static PyObject *
-_collections_deque___sizeof___impl(dequeobject *deque);
+deque___sizeof___impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque___sizeof__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque___sizeof__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque___sizeof___impl(deque);
+    return deque___sizeof___impl(deque);
 }
 
-PyDoc_STRVAR(_collections_deque___reversed____doc__,
+PyDoc_STRVAR(deque___reversed____doc__,
 "__reversed__($self, /)\n"
 "--\n"
 "\n"
 "Return a reverse iterator over the deque.");
 
-#define _COLLECTIONS_DEQUE___REVERSED___METHODDEF    \
-    {"__reversed__", (PyCFunction)_collections_deque___reversed__, METH_NOARGS, _collections_deque___reversed____doc__},
+#define DEQUE___REVERSED___METHODDEF    \
+    {"__reversed__", (PyCFunction)deque___reversed__, METH_NOARGS, deque___reversed____doc__},
 
 static PyObject *
-_collections_deque___reversed___impl(dequeobject *deque);
+deque___reversed___impl(dequeobject *deque);
 
 static PyObject *
-_collections_deque___reversed__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
+deque___reversed__(dequeobject *deque, PyObject *Py_UNUSED(ignored))
 {
-    return _collections_deque___reversed___impl(deque);
+    return deque___reversed___impl(deque);
 }
 
 PyDoc_STRVAR(_collections__count_elements__doc__,
@@ -490,4 +489,4 @@ tuplegetter_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
 exit:
     return return_value;
 }
-/*[clinic end generated code: output=8f7f860b44810b2c input=a9049054013a1b77]*/
+/*[clinic end generated code: output=a41b8a712bc7b6db input=a9049054013a1b77]*/

bedevere-app · 2024-01-17T08:16:33Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mpage · 2024-01-17T17:21:25Z

Ah, you are correct; that does work, though a little bit untraditional

This approach is suggested in the clinic guide for cases where there are a lot of functions that have the same type for self. Is this not a preferred approach?

erlend-aasland · 2024-01-17T17:29:39Z

Ah, you are correct; that does work, though a little bit untraditional

This approach is suggested in the clinic guide for cases where there are a lot of functions that have the same type for self. Is this not a preferred approach?

The approach is fine; I was just not used to see it applied to a self converter specifically (there is no precedent for that in the code base) 🙂

Use `value` everywhere (not `v` in some places)

Use the `as` clinic directive to minimize the changes to function names and arguments.

mpage · 2024-01-17T23:55:19Z

I have made the requested changes; please review again

Modules/_collectionsmodule.c

mpage · 2024-01-18T02:09:27Z

I have made the requested changes; please review again

aisk · 2024-01-18T06:46:18Z

Most arguments represent an element in the container changed its name to value, but deque.append and deque.appendleft still retain the name item.

Misc/NEWS.d/next/Core and Builtins/2024-01-11-22-58-45.gh-issue-112050.hDuvDW.rst

erlend-aasland · 2024-01-18T08:49:50Z

Most arguments represent an element in the container changed its name to value [...]

I think this is fine, as the inherited comparison slots also use value. The rest I think it is fine, since these are all positional-only methods. We can align those in another PR, if needed.

erlend-aasland · 2024-01-18T08:54:21Z

Thanks a lot, @mpage!

erlend-aasland · 2024-01-18T09:01:31Z

I'll wait a few days to give @rhettinger a chance to chime in, before merging this.

rhettinger · 2024-01-19T06:41:06Z

It may take me a while to get to this. The diff is huge and in places such as __init__, it removes some very carefully optimized code.

erlend-aasland · 2024-01-19T09:36:21Z

It may take me a while to get to this. The diff is huge and in places such as __init__, it removes some very carefully optimized code.

See the following issues for more information about nogil/free-threading and how Argument Clinic is helping carrying these changes through:

Argument Clinic: Add support for PEP 703's critical sections (Py_BEGIN_CRITICAL_SECTION()) #111903
PEP 703 -- Making the Global Interpreter Lock Optional in CPython #108219
- PEP-703

Regarding this particular PR and your attention to detail and optimised code:
Most of these methods are METH_NOARGS, METH_O or have positional-only params only; those are simple changes with no performance implications. Of course, the positive impact remains: improved text signatures and preparation for the @critical_section Argument Clinic directive.

The methods that stick out are: __init__ and index. AFAICS, the generated parsing code for index¹ is more performant than the existing parsing code². As for __init__, the existing code appears to be more performant than the generated code, but performance benchmarks show no significant slowdown.

Here are some quick timeit benchmarks (debug builds with -Og):

d.index()

main:

$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(0)"
5000000 loops, best of 5: 87.3 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(50)"
200000 loops, best of 5: 1.07 usec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(99)" 
100000 loops, best of 5: 2.04 usec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(5, 1, 10)"
1000000 loops, best of 5: 219 nsec per loop```
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(5, 1)"
2000000 loops, best of 5: 191 nsec per loop

This PR:

$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(0)"
5000000 loops, best of 5: 57.4 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(50)"
200000 loops, best of 5: 1.04 usec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(100)"
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(99)"
200000 loops, best of 5: 1.99 usec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(5, 1, 10)"
2000000 loops, best of 5: 174 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.index(5, 1)"
2000000 loops, best of 5: 155 nsec per loop

d.append(n), d.rotate(), d.rotate(n)

main:

$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.append(1)"
5000000 loops, best of 5: 52.6 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.rotate(1)"
5000000 loops, best of 5: 63.3 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.rotate()" 
5000000 loops, best of 5: 49.1 nsec per loop

This PR:

$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.append(1)"
5000000 loops, best of 5: 52.4 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.rotate(1)"
5000000 loops, best of 5: 63.3 nsec per loop
$ ./python.exe -m timeit -s "import collections; d = collections.deque(list(range(100)))" "d.rotate()"
5000000 loops, best of 5: 49.7 nsec per loop

__init__

main:

$ ./python.exe -m timeit -s "import collections; L = list(range(100))" "collections.deque(L, 50)"
100000 loops, best of 5: 2.11 usec per loop
$ ./python.exe -m timeit -s "import collections; L = list(range(100))" "collections.deque(L)"    
200000 loops, best of 5: 1.98 usec per loop

This PR:

$ ./python.exe -m timeit -s "import collections; L = list(range(100))" "collections.deque(L, 50)"
100000 loops, best of 5: 2.11 usec per loop
$ ./python.exe -m timeit -s "import collections; L = list(range(100))" "collections.deque(L)"
200000 loops, best of 5: 1.97 usec per loop

Using _PyArg_CheckPositional and _PyEval_SliceIndexNotNone ↩
Using _PyArg_ParseStack (which is a variadic function) ↩

serhiy-storchaka · 2024-01-19T12:38:09Z

See old issue:

Converting collections.deque methods to Argument Clinic #85533

erlend-aasland · 2024-01-19T13:17:08Z

See old issue:

Converting collections.deque methods to Argument Clinic #85533

Four years after, the motivation is different and we have new requirements wrt. PEP-703; this PR is one of many steps that prepare our code base for free-threading.

rhettinger · 2024-01-29T06:04:29Z

I won't get to this anytime soon, so feel free to proceed without me. That said, if a later review detects any user visible API change or performance regression, expect there to be an edit to fix it.

erlend-aasland · 2024-01-29T10:10:15Z

I won't get to this anytime soon, so feel free to proceed without me. That said, if a later review detects any user visible API change or performance regression, expect there to be an edit to fix it.

Thanks; I'll land this later today. We'll keep an eye on the benchmarks :) Thanks again for chiming in!

Misc/NEWS.d/next/Core and Builtins/2024-01-11-22-58-45.gh-issue-112050.hDuvDW.rst

Modules/_collectionsmodule.c

…e-112050.hDuvDW.rst

rhettinger · 2024-01-31T02:49:14Z

I gave this an initial pass today. It was nicely done. Thank you.

…13963)

mpage added 2 commits January 11, 2024 14:38

Convert most collections.deque methods to use clinic

2f9c90a

Convert most methods on collections.deque to use clinic. This will allow us to use clinic's `@critical_section` directive when making deques thread-safe for `--gil-disabled`, simplifying the implementation.

Merge remote-tracking branch 'upstream/main' into ft-deque-clinic

60b6346

bedevere-app bot mentioned this pull request Jan 11, 2024

Make collections.deque thread-safe in --disable-gil builds #112050

Closed

blurb-it bot and others added 3 commits January 11, 2024 22:58

📜🤖 Added by blurb_it.

eefd994

Fix error in docstring for popleft

991f686

We pop the leftmost, not the rightmost.

Make docstring for __init__ more closely resemble the original

37c4c05

The original version is ``` deque([iterable[, maxlen]]) --> deque object ``` The latter part seems redundant so has been omitted.

mpage marked this pull request as ready for review January 12, 2024 00:55

mpage requested a review from rhettinger as a code owner January 12, 2024 00:55

bedevere-app bot added the awaiting review label Jan 12, 2024

aisk reviewed Jan 14, 2024

8000 View reviewed changes

This comment was marked as outdated.

Sign in to view

serhiy-storchaka assigned rhettinger Jan 17, 2024

erlend-aasland requested changes Jan 17, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Jan 17, 2024

mpage added 3 commits January 17, 2024 10:53

Add missing periods in deque docstrings

35c16c2

Use consistent names for value params

875433b

Use `value` everywhere (not `v` in some places)

Drop the _collections_, it's cleaner

3600a40

Use the `as` clinic directive to minimize the changes to function names and arguments.

erlend-aasland reviewed Jan 18, 2024

View reviewed changes

Reuse existing argument names

b7bc0b2

aisk reviewed Jan 18, 2024

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2024-01-11-22-58-45.gh-issue-112050.hDuvDW.rst Outdated Show resolved Hide resolved

erlend-aasland approved these changes Jan 18, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Jan 18, 2024

erlend-aasland assigned erlend-aasland and unassigned rhettinger Jan 29, 2024

Merge branch 'main' into ft-deque-clinic

a306210

serhiy-storchaka self-requested a review January 29, 2024 10:21

serhiy-storchaka approved these changes Jan 29, 2024

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2024-01-11-22-58-45.gh-issue-112050.hDuvDW.rst Outdated Show resolved Hide resolved

Modules/_collectionsmodule.c Show resolved Hide resolved

Update Misc/NEWS.d/next/Core and Builtins/2024-01-11-22-58-45.gh-issu…

934fd2e

…e-112050.hDuvDW.rst

erlend-aasland enabled auto-merge (squash) January 29, 2024 14:43

erlend-aasland merged commit c87233f into python:main Jan 29, 2024

bedevere-app bot removed the awaiting merge label Jan 29, 2024

mpage deleted the ft-deque-clinic branch January 29, 2024 18:12

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-112050: Adapt collections.deque to Argument Clinic (python#1…

307afee

…13963)

Uh oh!

gh-112050: Convert most methods on collections.deque to use Argument Clinic #113963

gh-112050: Convert most methods on collections.deque to use Argument Clinic #113963

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Footnotes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!