gh-114329: Add `PyList_GetItemRef` function by colesbury · Pull Request #114504 · python/cpython (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation16 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Contributor

@colesbury colesbury commented

Jan 23, 2024

•

edited by github-actionsbot

The new PyList_GetItemRef is similar to PyList_GetItem, but returns a strong reference instead of a borrowed reference. Additionally, if the passed "list" object is not a list, the function sets a TypeError instead of calling PyErr_BadInternalCall().

Additionally, if the passed "list" object is not a list, the function sets a TypeError instead of calling PyErr_BadInternalCall().

When I proposed such change, @serhiy-storchaka asked me to stick to RuntimeError, but I don't recall why :-D @serhiy-storchaka, do you prefer RuntimeError or TypeError?

:exc:`TypeError` exception.

This behaves like :c:func:`PyList_GetItem`, but returns a
:term:`strong reference` instead of a :term:`borrowed reference`.

Choose a reason for hiding this comment

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

I suggest to make this function the new reference, and document PyList_GetItem() as: "Similar to PyList_GetItemRef(), but return a borrowed reference".

@@ -112,6 +112,15 @@ def test_list_get_item(self):
# CRASHES get_item(21, 2)
# CRASHES get_item(NULL, 1)

def test_list_get_item_ref(self):

Choose a reason for hiding this comment

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

Can you share most code between test_list_get_item() and test_list_get_item_ref()?

Comment on lines 908 to 909

[function.PyList_GetItemRef]
added = '3.13'

Choose a reason for hiding this comment

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

Move it to the end of the file.

When I proposed such change, @serhiy-storchaka asked me to stick to RuntimeError, but I don't recall why :-D @serhiy-storchaka, do you prefer RuntimeError or TypeError?

Is it RuntimeError or SystemError?

The only discussed difference was returning a strong reference instead of a weak reference. If we want to change also the exception type, it should be a separate discussion.

Most concrete type C API raises SystemError if they are called with wrong type. It is considered a programming error, not a data error.

Most concrete type C API raises SystemError if they are called with wrong type. It is considered a programming error, not a data error.

I suggest to document that the function "parameter must be list" , without mentioning the exact exception type.

I've removed the sentence mentioning TypeError from the PyList_GetItemRef documentation. I don't think saying that "parameter must be list" without describing the error return is helpful. That merely states the obvious and risks being confusing: it raises the question of why, out of all the functions that must take a list, only this one says "parameter must be list".