Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-111138: Add PyList_Extend() and PyList_Clear() functions#111862
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Nov 8, 2023 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Nov 8, 2023
This change keeps the internal C API: |
vstinner commented Nov 8, 2023
Oh, these functions should be added to the limited C API. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Objects/listobject.c Outdated
| } | ||
| PyObject* | ||
| _PyList_Extend(PyListObject*self, PyObject*iterable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove _PyList_Extend and inline list_extend_method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to not remove _PyList_Extend() in the same PR, since remove private functions seems to be a controversial topic. If it's removed, I prefer to remove it in a separated PR. PyList_Extend() returns an int, whereas _PyList_Extend() returns an object.
I modified the code to remove list_extend_method().
Uh oh!
There was an error while loading. Please reload this page.
Doc/c-api/list.rst Outdated
| .. c:function::intPyList_Clear(PyObject *list) | ||
| Remove all items from *list*. Similar to: | ||
| ``PyList_SetSlice(L, 0, PY_SSIZE_T_MAX, NULL)``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``PyList_SetSlice(L, 0, PY_SSIZE_T_MAX, NULL)``. | |
| ``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)``. |
What does it mean to be similar? Does it have exactly the same effect? If not, what is the difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyList_Clear(list) is the same as PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL). Internally, both end up calling list_clear(): same code path.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Nov 9, 2023
@serhiy-storchaka@eendebakpt: I addressed your review. Please review updated PR. I also added a note about the corner case of finalizer modifying the list, and I even added tests on this corner case. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Nov 9, 2023
I removed notes form the documentation about finalizers which can modify the list while PyList_Clear() and PyList_SetSlice(). |
serhiy-storchaka commented Nov 9, 2023
I suggest to change exceptions to SystemError. It is easier to change from SystemError to other type if needed than in the other direction. |
vstinner commented Nov 9, 2023
Ok, done. |
vstinner commented Nov 9, 2023
Done: I added the functions to the limited C API version 3.13. |
vstinner commented Nov 9, 2023
@encukou: I added these 2 "new" functions to the limited C API. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a value in the note about possible non-emptiness of the list after clear(), but it can be added later, and perhaps in different form. The false assumption that the list is empty after clear() can lead to more severe bugs in C than in Python. It is not only about modifying the list in the item's destructor, it is mainly about releasing the GIL and modifying the list in other thread.
I left some suggestions for the documentation. I also think that it is better to move the documentation for new functions just past the documentation for PyList_SetSlice. The order here is not lexicographical, but semantical.
The rest LGTM.
Doc/c-api/list.rst Outdated
| Remove all items from *list*. Similar to: | ||
| ``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Remove all items from *list*. Similar to: | |
| ``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)``. | |
| Remove all items from *list*. | |
| This is the same as ``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)`` and | |
| analogous to ``list.clear()`` or ``del list[:]``. |
Doc/c-api/list.rst Outdated
| Extend *list* with the contents of *iterable*. Similar to: | ||
| ``PyList_SetSlice(list, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, iterable)``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Extend *list* with the contents of *iterable*. Similar to: | |
| ``PyList_SetSlice(list, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, iterable)``. | |
| Extend *list* with the contents of *iterable*. | |
| This is the same as ``PyList_SetSlice(list, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, iterable)`` | |
| and analogous to ``list.extend(iterable)`` or ``list += iterable``. |
vstinner commented Nov 10, 2023
The problem is that Python API is also affected. Other mutable and mapping types are affected. Should we add note in all Python and C functions which might be affected by the issue? I added the note since it exists in the C implementation. But I concur with Antoine, it's not worth it to confuse users about this corner case. |
vstinner commented Nov 10, 2023
I applied your suggestions.
Right, it's semantical. I moved PyList_Clear(), but left Extend after Append. For me, they go together :-) Maybe functions should just be sorted :-) |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You moved PyList_Clear() after PyList_SET_ITEM(), not after PyList_SetSlice().
The reason of moving PyList_Clear() and PyList_Extend() after PyList_SetSlice() is that functions that work with a single item go first, followed by functions that work with multiple items, and that both PyList_Clear() and PyList_Extend() are convenient functions for particular use cases of PyList_SetSlice().
vstinner commented Nov 10, 2023
Ok, I moved the functions after PyList_SetSlice() in the doc. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
encukou commented Nov 13, 2023
Please wait until we have a better process for adding to the limited API -- e.g. until the WG is formed and can discuss this. |
vstinner commented Nov 13, 2023
I modified the PR just before you added this comment to not add these functions to the stable ABI / limited C API :-) |
* Split list_extend() into two sub-functions: list_extend_fast() and list_extend_iter(). * list_inplace_concat() no longer has to call Py_DECREF() on the list_extend() result, since list_extend() now returns an int.
vstinner commented Nov 13, 2023
I also moved the definition to Include/cpython/. |
…thon#111862) * Split list_extend() into two sub-functions: list_extend_fast() and list_extend_iter(). * list_inplace_concat() no longer has to call Py_DECREF() on the list_extend() result, since list_extend() now returns an int.
…thon#111862) * Split list_extend() into two sub-functions: list_extend_fast() and list_extend_iter(). * list_inplace_concat() no longer has to call Py_DECREF() on the list_extend() result, since list_extend() now returns an int.
📚 Documentation preview 📚: https://cpython-previews--111862.org.readthedocs.build/