Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The `PyDict_SetDefaultRef` function is similar to `PyDict_SetDefault`,but returns a strong reference through the optional `**result` pointerinstead of a borrowed reference.
cc@zooba,@encukou, and@serhiy-storchaka in case you are interested in this |
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. |
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
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.
With nogil, the latter is incorrect. The reference can become invalid (and the object deallocated) between the |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
As@encukou wrote, returning a strong reference is important in nogil builds and can't be safely simulated with a subsequent I'll put up a subsequent PR that replaces internal |
@serhiy-storchaka - here is an example PR that changes usages:#112211. When this PR lands, I'll rebase that PR on top of it. |
Are there any outstanding concerns with this change@encukou@serhiy-storchaka? |
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.
Thank you for#112211 (yet one nice number!).
Please add a What's New entry. The rest LGTM.
I've added the "What's New" entry |
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.
@serhiy-storchaka - would you please merge this? |
Provided for consideration of the C API WG:capi-workgroup/decisions#4. |
Looks like the C API WG approved the API.@serhiy-storchaka, can we merge this PR now? |
One more issue needs to be resolved. Wait another week. 😉 |
This comment was marked as resolved.
This comment was marked as resolved.
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! |
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>
Uh oh!
There was an error while loading.Please reload this page.
The
PyDict_SetDefaultRef
function is similar toPyDict_SetDefault
, but returns a strong reference through the optional**result
pointer instead of a borrowed reference.PyDict_SetDefault
that returns a new reference (instead of a borrowed reference) #112066📚 Documentation preview 📚:https://cpython-previews--112123.org.readthedocs.build/