◐ Shell
reader mode source ↗
Skip to content

gh-111138: Add PyList_Extend() and PyList_Clear() functions#111862

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:list_extend
Nov 13, 2023
Merged

gh-111138: Add PyList_Extend() and PyList_Clear() functions#111862
vstinner merged 1 commit into
python:mainfrom
vstinner:list_extend

Conversation

@vstinner

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

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.

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

@bedevere-app bedevere-app Bot mentioned this pull request Nov 8, 2023
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 *);

@vstinner

vstinner commented Nov 8, 2023

Copy link
Copy Markdown
Member Author

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

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

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

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

@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

I suggest to change exceptions to SystemError.

Ok, done.

@vstinner vstinner requested review from a team and encukou as code owners November 9, 2023 22:17
@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.

@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

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

Choose a reason for hiding this comment

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

Hide comment

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.

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

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

serhiy-storchaka
serhiy-storchaka reviewed Nov 10, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

vstinner commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

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

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

Choose a reason for hiding this comment

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

Hide comment

LGTM.

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

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

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

@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
@encukou encukou mentioned this pull request Nov 13, 2023
Open
@vstinner vstinner mentioned this pull request Nov 13, 2023
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

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