gh-111545: Add Py_HashPointer() function#112096
Conversation
eaf1414 to
3a0752a
Compare
|
I checked how
As expected, the I used For that, I made the following change. The Change: diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c
index 2d888e0ae2c..a08b3dfb168 100644
--- a/Modules/_testinternalcapi.c
+++ b/Modules/_testinternalcapi.c
@@ -290,6 +290,23 @@ test_rotateright_uintptr(PyObject *self, PyObject *Py_UNUSED(args))
}
+static PyObject*
+rotateright_uintptr(PyObject *self, PyObject *args)
+{
+ PyObject *obj;
+ int bits;
+ if (!PyArg_ParseTuple(args, "O!i", &PyLong_Type, &obj, &bits)) {
+ return NULL;
+ }
+ unsigned long long x = PyLong_AsUnsignedLongLong(obj);
+ if (x == (unsigned long long)-1 && PyErr_Occurred()) {
+ return NULL;
+ }
+ uintptr_t y = _Py_rotateright_uintptr((uintptr_t)x, bits);
+ return PyLong_FromUnsignedLongLong(y);
+}
+
+
#define TO_PTR(ch) ((void*)(uintptr_t)ch)
#define FROM_PTR(ptr) ((uintptr_t)ptr)
#define VALUE(key) (1 + ((int)(key) - 'a'))
@@ -1689,6 +1706,7 @@ static PyMethodDef module_functions[] = {
{"test_popcount", test_popcount, METH_NOARGS},
{"test_bit_length", test_bit_length, METH_NOARGS},
{"test_rotateright_uintptr", test_rotateright_uintptr, METH_NOARGS},
+ {"rotateright_uintptr", rotateright_uintptr, METH_VARARGS},
{"test_hashtable", test_hashtable, METH_NOARGS},
{"get_config", test_get_config, METH_NOARGS},
{"set_config", test_set_config, METH_O}, |
5b19577 to
d132eb6
Compare
992c39e to
f63b726
Compare
Adding |
f63b726 to
fb08414
Compare
|
Changes:
|
fb08414 to
ffbdae1
Compare
|
@encukou: Please review the updated PR. |
ffbdae1 to
cd8849b
Compare
|
@serhiy-storchaka: Do you want to review this change? |
136b536 to
8031210
Compare
|
I rebased the PR on main to get a NoGIL fix. |
PEP 731 says:
Are you suggesting that the C API cannot be modified anymore unless whole C API Working Group approve a change? Or are you suggesting that the C API Working Group must review all C API additions? I don't think that was the plan. Well, the plan is not well defined. |
No, I'm saying that the others might disagree -- and that I don't think it's very likely that they will disagree. |
zooba
left a comment
There was a problem hiding this comment.
No concerns from me, other than the lack of docs.
Adding new C API should include the WG up until we've developed and agreed upon the guidelines for adding new C API. Without those, yeah, we should all get pinged for any new APIs. (And if that takes up all our time and we don't get a chance to develop guidelines, so be it...)
Doc/c-api/hash.rst
Outdated
|
|
||
| .. c:function:: Py_hash_t Py_HashPointer(const void *ptr) | ||
|
|
||
| Hash a pointer. |
There was a problem hiding this comment.
Can we add a bit more text to clarify that it ensures the pointer itself is usable as a hash value, and does not even attempt to access the memory the pointer refers to (let alone trying to call __hash__ or similar)?
There was a problem hiding this comment.
I completed the doc. Is it more explicit? I clarified that only the pointer value is hashed.
* Implement _Py_HashPointerRaw() as a static inline function. * Add Py_HashPointer() tests to test_capi.test_hash. * Keep _Py_HashPointer() function as an alias to Py_HashPointer().
8031210 to
5415271
Compare
@encukou created capi-workgroup/decisions#1 |
| ``uintptr_t`` internally). The pointer is not dereferenced. | ||
|
|
||
| The function cannot fail: it cannot return ``-1``. | ||
|
|
There was a problem hiding this comment.
I'm missing some mention of the use case for this (not necessarily here). The issue talks about _Py_HashDouble, there is no mention of Py_HashPointer.
Proposed Moreover, I removed the private function
Finally, igraph-0.11.2 contains a copy of the private |
|
I merged my PR. The API was approved by the C API Working Group: capi-workgroup/decisions#1 |
* Implement _Py_HashPointerRaw() as a static inline function. * Add Py_HashPointer() tests to test_capi.test_hash. * Keep _Py_HashPointer() function as an alias to Py_HashPointer().
* Implement _Py_HashPointerRaw() as a static inline function. * Add Py_HashPointer() tests to test_capi.test_hash. * Keep _Py_HashPointer() function as an alias to Py_HashPointer().
_Py_HashDoublepublic again as "unstable" API #111545📚 Documentation preview 📚: https://cpython-previews--112096.org.readthedocs.build/