Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 1 commit intopython:mainfromvstinner:list_extend
Nov 13, 2023

Conversation

vstinner
Copy link
Member

@vstinnervstinner commentedNov 8, 2023•
edited by github-actionsbot
Loading

  • 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/

serhiy-storchaka reacted with thumbs up emoji
@vstinner
Copy link
MemberAuthor

This change keeps the internal C API:

Include/internal/pycore_list.h: extern PyObject* _PyList_Extend(PyListObject *, PyObject *);

@vstinner
Copy link
MemberAuthor

Oh, these functions should be added to the limited C API.

py_list_extend(PyListObject *self, PyObject *iterable)
/*[clinic end generated code: output=b8e0bff0ceae2abd input=9a8376a8633ed3ba]*/
{
return list_extend_method(self, iterable);
}

PyObject *
_PyList_Extend(PyListObject *self, PyObject *iterable)

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 inlinelist_extend_method.

Copy link
MemberAuthor

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().

.. c:function:: int PyList_Clear(PyObject *list)

Remove all items from *list*. Similar to:
``PyList_SetSlice(L, 0, PY_SSIZE_T_MAX, NULL)``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
``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

Copy link
MemberAuthor

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 asPyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL). Internally, both end up callinglist_clear(): same code path.

@vstinner
Copy link
MemberAuthor

@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.

@vstinner
Copy link
MemberAuthor

I removed notes form the documentation about finalizers which can modify the list while PyList_Clear() and PyList_SetSlice().

@serhiy-storchaka
Copy link
Member

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
Copy link
MemberAuthor

I suggest to change exceptions to SystemError.

Ok, done.

@vstinner
Copy link
MemberAuthor

Oh, these functions should be added to the limited C API.

Done: I added the functions to the limited C API version 3.13.

@vstinner
Copy link
MemberAuthor

@encukou: I added these 2 "new" functions to the limited C API.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a 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 forPyList_SetSlice. The order here is not lexicographical, but semantical.

The rest LGTM.

Comment on lines 56 to 57
Remove all items from *list*. Similar to:
``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)``.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
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[:]``.

Comment on lines 128 to 129
Extend *list* with the contents of *iterable*. Similar to:
``PyList_SetSlice(list, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, iterable)``.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
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
Copy link
MemberAuthor

I think there was a value in the note about possible non-emptiness of the list after clear()

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
Copy link
MemberAuthor

I left some suggestions for the documentation.

I applied your suggestions.

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.

Right, it's semantical. I moved PyList_Clear(), but left Extend after Append. For me, they go together :-) Maybe functions should just be sorted :-)

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You movedPyList_Clear() afterPyList_SET_ITEM(), not afterPyList_SetSlice().

The reason of movingPyList_Clear() andPyList_Extend() afterPyList_SetSlice() is that functions that work with a single item go first, followed by functions that work with multiple items, and that bothPyList_Clear() andPyList_Extend() are convenient functions for particular use cases ofPyList_SetSlice().

@vstinner
Copy link
MemberAuthor

Ok, I moved the functions after PyList_SetSlice() in the doc.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM.

@encukou
Copy link
Member

@encukou: I added these 2 "new" functions to the limited C API.

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
Copy link
MemberAuthor

@encukou:

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.

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
Copy link
MemberAuthor

I modified the PR just before you added this comment to not add these functions to the stable ABI / limited C API :-)

I also moved the definition to Include/cpython/.

@vstinnervstinnerenabled auto-merge (squash)November 13, 2023 16:10
@vstinnervstinner merged commitbabb787 intopython:mainNov 13, 2023
@vstinnervstinner deleted the list_extend branchNovember 13, 2023 16:14
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
…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.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
…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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@eendebakpteendebakpteendebakpt left review comments

@pitroupitroupitrou left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@encukouencukouAwaiting requested review from encukou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@vstinner@serhiy-storchaka@encukou@eendebakpt@pitrou
[8]ページ先頭
©2009-2025 Movatter.jp