GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-111545: Add Py_HashPointer() function by vstinner · Pull Request #112096 · python/cpython Conversation
|
|
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}, |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
5b19577 to
d132eb6
Compare
992c39e to
f63b726
Compare
There was an error while loading. Please reload this page.
Adding |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
f63b726 to
fb08414
Compare
|
Changes:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
fb08414 to
ffbdae1
Compare
|
@encukou: Please review the updated PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
ffbdae1 to
cd8849b
Compare
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, thank you for adding it!
I hope Serhiy is OK with this iteration. (And that the rest of the C-API WG won't have major objections, but Idon't think that's likely.)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@serhiy-storchaka: Do you want to review this change? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
136b536 to
8031210
Compare
|
I rebased the PR on main to get a NoGIL fix. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
No, I'm saying that the others might disagree -- and that I don't think it's very likely that they will disagree. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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...)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| .. c:function:: Py_hash_t Py_HashPointer(const void *ptr) | ||
|
|
||
| Hash a pointer. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I completed the doc. Is it more explicit? I clarified that only the pointer value is hashed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ``uintptr_t`` internally). The pointer is not dereferenced. | ||
|
|
||
| The function cannot fail: it cannot return ``-1``. | ||
|
|
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Proposed Moreover, I removed the private function
Finally, igraph-0.11.2 contains a copy of the private |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I merged my PR. The API was approved by the C API Working Group: capi-workgroup/decisions#1 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* 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().
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.