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-112066: AddPyDict_SetDefaultRef function.#112123

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
colesbury merged 5 commits intopython:mainfromcolesbury:PyDict_SetDefaultRef
Feb 6, 2024

Conversation

colesbury
Copy link
Contributor

@colesburycolesbury commentedNov 15, 2023
edited by github-actionsbot
Loading

ThePyDict_SetDefaultRef function is similar toPyDict_SetDefault, but returns a strong reference through the optional**result pointer instead of a borrowed reference.


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

The `PyDict_SetDefaultRef` function is similar to `PyDict_SetDefault`,but returns a strong reference through the optional `**result` pointerinstead of a borrowed reference.
@colesbury
Copy link
ContributorAuthor

cc@zooba,@encukou, and@serhiy-storchaka in case you are interested in this

@zooba
Copy link
Member

My only comment is the same as for the recently added pop functions: we should not use these functions as precedent for their API design (specifically how the return value is handled). When we come to decide which approach we want to standardise for the C API in general, we aren't going to be forced into choosing this design just because it was used during the period between deciding-to-decide and actually deciding.

But since there's a chance that the other recently added API could also become "warts", best to align with those since they're all on the same type. There's nothing gained by deliberate inconsistency when the functions show up right next to each other.

colesbury reacted with thumbs up emoji

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.

Very nice. The code looks good.

But I worry that all exit branches simply addPy_NewRef() forPyDict_SetDefaultRef(). It means thatPyDict_SetDefaultRef() is not principally different fromPyDict_SetDefault() combined withPy_XNewRef(). The question arises about the need to add a new function.

Could you please create a separate PR that replaces allPyDict_SetDefault() withPyDict_SetDefaultRef() to show what benefit it could bring.

@encukou
Copy link
Member

we should not use these functions as precedent for their API design

IMO, if the C-API WG is formed and decides soon enough, this API and all its uses in CPython might be adjusted. That might affect in-flight PRs.


PyDict_SetDefaultRef() is not principally different fromPyDict_SetDefault() combined withPy_XNewRef()

With nogil, the latter is incorrect. The reference can become invalid (and the object deallocated) between thePyDict_SetDefault() andPy_XNewRef() calls.
(That doesn't apply if you know no other threads can mutate the dict, but future changes can easily invalidate assumptions like that.)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@colesbury
Copy link
ContributorAuthor

As@encukou wrote, returning a strong reference is important in nogil builds and can't be safely simulated with a subsequentPy_INCREF().

I'll put up a subsequent PR that replaces internalPyDict_SetDefault calls withPyDict_SetDefaultRef.

@colesbury
Copy link
ContributorAuthor

@serhiy-storchaka - here is an example PR that changes usages:#112211. When this PR lands, I'll rebase that PR on top of it.

@colesbury
Copy link
ContributorAuthor

Are there any outstanding concerns with this change@encukou@serhiy-storchaka?

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.

Thank you for#112211 (yet one nice number!).

Please add a What's New entry. The rest LGTM.

colesbury reacted with thumbs up emoji
@colesbury
Copy link
ContributorAuthor

I've added the "What's New" entry

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.

@colesbury
Copy link
ContributorAuthor

@serhiy-storchaka - would you please merge this?

@serhiy-storchaka
Copy link
Member

Provided for consideration of the C API WG:capi-workgroup/decisions#4.

@colesbury
Copy link
ContributorAuthor

Looks like the C API WG approved the API.@serhiy-storchaka, can we merge this PR now?

erlend-aasland reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

One more issue needs to be resolved. Wait another week. 😉

erlend-aasland reacted with hooray emoji

@erlend-aasland

This comment was marked as resolved.

@serhiy-storchaka
Copy link
Member

The issue I was talking about was the ongoing vote to promote@colesbury as a core developer. I think it would be nice if Sam merge his PR himself. Welcome on the board!

erlend-aasland and colesbury reacted with hooray emojierlend-aasland, AlexWaygood, and carljm reacted with heart emoji

@colesburycolesbury merged commitde61d4b intopython:mainFeb 6, 2024
@colesburycolesbury deleted the PyDict_SetDefaultRef branchFebruary 6, 2024 16:36
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull requestFeb 14, 2024
The `PyDict_SetDefaultRef` function is similar to `PyDict_SetDefault`,but returns a strong reference through the optional `**result` pointerinstead of a borrowed reference.Co-authored-by: Petr Viktorin <encukou@gmail.com>
@colesburycolesbury removed their assignmentJul 19, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@encukouencukouencukou left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@vstinnervstinnerAwaiting requested review from vstinner

@methanemethaneAwaiting requested review from methanemethane is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees
No one assigned
Labels
3.13bugs and security fixestopic-C-APItopic-free-threadingtype-featureA feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@colesbury@zooba@encukou@serhiy-storchaka@erlend-aasland

[8]ページ先頭

©2009-2025 Movatter.jp