capi-workgroup / decisions

Discussion and voting on specific issues
8 stars 1 forks source link

Add PyDict_SetDefaultRef() function #4

Closed serhiy-storchaka closed 10 months ago

serhiy-storchaka commented 11 months ago

PR: https://github.com/python/cpython/pull/112123

int PyDict_SetDefaultRef(PyObject *p, PyObject *key, PyObject *default_value, PyObject **result)

It is similar to existing API PyObject *PyDict_SetDefault(PyObject *p, PyObject *key, PyObject *default_value), but have slightly different interface and returns a strong reference.

With the GIL it is virtually equivalent to PyDict_SetDefault() followed by Py_XINCREF():

int PyDict_SetDefaultRef(PyObject *p, PyObject *key, PyObject *default_value, PyObject **result)
{
    *result = PyDict_SetDefault(p, key, default_value);
    Py_XINCREF(*result);
    return *result ? 0 : -1;
}

But it holds a strong reference internally, so it has larger chance to be made thread-safe in GIL-less build (this still requires a lot of work and may be not worth to do). PyDict_SetDefault() is not thread-safe without the GIL in principle, because the weak reference can became invalid between PyDict_SetDefault() and Py_XINCREF().


encukou commented 10 months ago

Note the clarification in the docs:

For clarity: if you have a strong reference to default_value before calling this function, then after it returns, you hold a strong reference to both default_value and *result (if it's not NULL). These may refer to the same object: in that case you hold two separate references to it.

+1 from me; I think this is a good precedent for “setdefault”-like API.

vstinner commented 10 months ago

+1 to add this function.

Functions returning borrowed references are always error prone. Adding this function fix yet another function in my list of functions returning borrowed references: https://pythoncapi.readthedocs.io/bad_api.html#functions

zooba commented 10 months ago

+1 from me; I think this is a good precedent for “setdefault”-like API.

Agreed. This is the precedent I like.

gvanrossum commented 10 months ago

+1 from me too -- this is in line with PyDict_GetItemRef() and friends.

vstinner commented 10 months ago

Ping @iritkatriel: what's your opinion on this API?

iritkatriel commented 10 months ago

Ping @iritkatriel: what's your opinion on this API?

Yes, I agree.

vstinner commented 10 months ago

@serhiy-storchaka: Congrats, your API is approved, you can go ahead.