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-114329: Add `PyList_GetItemRef` function by colesbury · Pull Request #114504 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Jan 23, 2024 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

  • Issue: Add PyList_GetItemRef, a variant of PyList_GetItem that returns a strong reference #114329

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@colesbury
pythongh-114329: Add PyList_GetItemRef function
8f96d08 
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()`.
@bedevere-app bedevere-app Bot mentioned this pull request Jan 23, 2024
Add PyList_GetItemRef, a variant of PyList_GetItem that returns a strong reference #114329
Closed
colesbury added 2 commits January 23, 2024 21:52
@colesbury
Run regen-limited-abi
8c92a40
@colesbury
Add PyList_GetItemRef to refcounts.dat
1761633
@colesbury
colesbury marked this pull request as ready for review January 29, 2024 21:08
@colesbury
colesbury requested review from a team and encukou as code owners January 29, 2024 21:08
@bedevere-app bedevere-app Bot added the awaiting review label Jan 29, 2024
@colesbury colesbury mentioned this pull request Jan 29, 2024
Add PyList_GetItemRef() (a variant of PyList_GetItem that returns a strong reference) capi-workgroup/decisions#9
Closed
@colesbury
colesbury requested a review from corona10 January 29, 2024 21:19
@corona10 corona10 self-assigned this Jan 30, 2024
@vstinner

vstinner commented Jan 30, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Jan 30, 2024
View reviewed changes
Comment thread Doc/c-api/list.rst Outdated
:exc:`TypeError` exception.

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

@vstinner vstinner Jan 30, 2024

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 make this function the new reference, and document PyList_GetItem() as: "Similar to PyList_GetItemRef(), but return a borrowed reference".

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 Outdated
# CRASHES get_item(21, 2)
# CRASHES get_item(NULL, 1)

def test_list_get_item_ref(self):

@vstinner vstinner Jan 30, 2024

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.

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

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
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Jan 30, 2024
View reviewed changes
Comment thread Misc/stable_abi.toml Outdated
Comment on lines +908 to +909
[function.PyList_GetItemRef]
added = '3.13'

@serhiy-storchaka serhiy-storchaka Jan 30, 2024

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.

Move it to the end of the file.

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka commented Jan 30, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

vstinner commented Jan 30, 2024

Copy link
Copy Markdown
Member

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.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

colesbury commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

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

The TypeError behavior is explicitly stated in the C API WG discussion of this API. See also capi-workgroup/api-evolution#29 (comment) and PyWeakref_GetRef().
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Changes from review
b91363f
@colesbury

colesbury commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

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".
erlend-aasland and encukou reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Merge branch 'main' into pythongh-114329-list
c045495
vstinner
vstinner approved these changes Jan 31, 2024
View reviewed changes

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

The TypeError behavior is explicitly stated in the C API WG capi-workgroup/decisions#9 (comment). See also capi-workgroup/api-evolution#29 (comment) and PyWeakref_GetRef().

Oh ok, I didn't notice that TypeError was explicitly mentioned.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 31, 2024
@corona10 corona10 mentioned this pull request Feb 1, 2024
gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe #114843
Merged
corona10
corona10 approved these changes Feb 1, 2024
View reviewed changes

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

I will merge the PR if @serhiy-storchaka approve.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
encukou
encukou approved these changes Feb 1, 2024
View reviewed changes

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

If the WG does discuss the exception type, it'll take some time.
Let's merge this, the type can be changed later

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland and corona10 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
erlend-aasland
erlend-aasland approved these changes Feb 1, 2024
View reviewed changes
Comment thread Doc/c-api/list.rst Resolved

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@colesbury @erlend-aasland
Apply suggestions from code review
dae4945 
Co-authored-by: Erlend E. Aasland <[email protected]>
@encukou
encukou merged commit d0f1307 into python:main Feb 2, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 2, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@colesbury @aisk
pythongh-114329: Add PyList_GetItemRef function (pythonGH-114504)
445e147 
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()`.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@colesbury @fsc-eriker
pythongh-114329: Add PyList_GetItemRef function (pythonGH-114504)
d3c77d1 
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()`.
@colesbury
colesbury deleted the gh-114329-list branch April 8, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@vstinner vstinner vstinner approved these changes
@encukou encukou encukou approved these changes
@corona10 corona10 corona10 approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

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

Assignees

@corona10 corona10

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.

6 participants

@colesbury @vstinner @serhiy-storchaka @encukou @corona10 @erlend-aasland
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.