capi-workgroup / decisions

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

Add `Py_hash_t Py_HashDouble(double, PyObject *obj)` function #10

Closed vstinner closed 4 months ago

vstinner commented 5 months ago

API: Py_hash_t PyHash_Double(double value, PyObject *obj)

Pull request: https://github.com/python/cpython/pull/112095

Differences with the existing private API Py_hash_t _Py_HashDouble(PyObject *inst, double v):

Accepting NULL is a trade off with alternative APIs which have no PyObject* argument, proposed previously:

The PyObject* parameter avoids asking the caller to implement their own code for the not-a-number case using Py_HashPointer(), PyObject_GenericHash(), or something else. Python 3.10 changed hash(float) to return hash(id(obj)) if the float is not-a-number. Writing such code working on all Python implementation can be tricky. Proposed Py_hash_t PyHash_Double(double value, PyObject *obj) API hides the implementation details, so using this proposed API is more portable.

Main user of the existing private _Py_HashDouble() function: numpy, link to code.

#if PY_VERSION_HEX > 0x030a00a6
#define Npy_HashDouble _Py_HashDouble
#else
static inline Py_hash_t
Npy_HashDouble(PyObject *NPY_UNUSED(identity), double val)
{
    return _Py_HashDouble(val);
}
#endif

static npy_hash_t
@lname@_arrtype_hash(PyObject *obj)
{
    return Npy_HashDouble(obj, (double)PyArrayScalar_VAL(obj, @name@));
}

More background on the Py_HashDouble() API in the previous issue: https://github.com/capi-workgroup/decisions/issues/2

Alternative: give up trying to add a public C API and comes back to Python 3.12 status quo with the private API Py_hash_t _Py_HashDouble(PyObject *inst, double v).

Votes:

gvanrossum commented 5 months ago

As I said before in the other issue, I have soured on "fixing" this. I'd rather keep the status quo. If we have to do something (but I don't think we do), the proposed API looks like it's just removing the leading _, so that would be my second choice. But I'm not ticking the box yet.

encukou commented 4 months ago

IMO, this API is too tightly tied to one specific use in NumPy (and to CPython's internal use). It trades one implementation detail (NaN handling) for another (using a PyObject* container/wrapper).

I'd be OK with exposing it as PyUnstable_, but i don't think it's a good fit for the general C API. I'd be OK with the status quo (_Py_HashDouble, with an underscore), but I'd treat that more as relieving pressure on us to fix low-priority issues, rather than a long-term fix.

gvanrossum commented 4 months ago

I'm taking an executive decision and closing this issue for lack of progress.

encukou commented 4 months ago

Does that mean we should revert to 3.12 (_Py_HashDouble), or take no action (remove the function without replacement)?

gvanrossum commented 4 months ago

Revert to 3.12.