GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111138: Add PyList_Extend() and PyList_Clear() functions by vstinner · Pull Request #111862 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Nov 8, 2023 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • 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.
  • Issue: C API: Consider adding a public PyList_Extend() function #111138

📚 Documentation preview 📚: https://cpython-previews--111862.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 8, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Nov 8, 2023
C API: Consider adding a public PyList_Extend() function #111138
Closed
@vstinner

vstinner commented Nov 8, 2023

Copy link
Copy Markdown
Member Author

This change keeps the internal C API:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 8, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 8, 2023
View reviewed changes
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Outdated
}

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

@serhiy-storchaka serhiy-storchaka Nov 8, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
eendebakpt
eendebakpt reviewed Nov 8, 2023
View reviewed changes
Comment thread Doc/c-api/list.rst Outdated
.. c:function:: int PyList_Clear(PyObject *list)

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

@eendebakpt eendebakpt Nov 8, 2023

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 9, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/list.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou reviewed Nov 9, 2023
View reviewed changes
Comment thread Doc/c-api/list.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou reviewed Nov 9, 2023
View reviewed changes
Comment thread Doc/c-api/list.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou reviewed Nov 9, 2023
View reviewed changes
Comment thread Doc/c-api/list.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Nov 9, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

I suggest to change exceptions to SystemError.

Ok, done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner requested review from a team and encukou as code owners November 9, 2023 22:17
@vstinner
vstinner force-pushed the list_extend branch from b22290e to 0a59d09 Compare November 9, 2023 22:18
@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 10, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/list.rst Outdated
Comment on lines +56 to +57
Remove all items from *list*. Similar to:
``PyList_SetSlice(list, 0, PY_SSIZE_T_MAX, NULL)``.

@serhiy-storchaka serhiy-storchaka Nov 10, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/list.rst Outdated
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)``.

@serhiy-storchaka serhiy-storchaka Nov 10, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 10, 2023
@vstinner

vstinner commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

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 :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 10, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 10, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@encukou

encukou commented Nov 13, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

@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 :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
pythongh-111138: Add PyList_Extend() and PyList_Clear() functions
751158a 
* 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
vstinner force-pushed the list_extend branch from 32f6976 to 751158a Compare November 13, 2023 15:46
@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

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/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner enabled auto-merge (squash) November 13, 2023 16:10
@vstinner
vstinner merged commit babb787 into python:main Nov 13, 2023
@vstinner
vstinner deleted the list_extend branch November 13, 2023 16:14
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 13, 2023
@encukou encukou mentioned this pull request Nov 13, 2023
Scope of the limited API capi-workgroup/api-evolution#41
Open
@vstinner vstinner mentioned this pull request Nov 13, 2023
Set guidelines to add APIs to the limited C API and to the stable ABI capi-workgroup/api-evolution#42
Open
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @aisk
pythongh-111138: Add PyList_Extend() and PyList_Clear() functions (py…
a5bb657 
…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 request Sep 2, 2024
@vstinner @Glyphack
pythongh-111138: Add PyList_Extend() and PyList_Clear() functions (py…
2b3832f 
…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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments
@pitrou pitrou pitrou left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@encukou encukou Awaiting requested review from encukou

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vstinner @serhiy-storchaka @encukou @eendebakpt @pitrou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.