Don't confuse uintptr_t and Py_ssize_t. #12569
Conversation
src/py_converters.cpp
Outdated
| int convert_voidptr(PyObject *obj, void *p) | ||
| { | ||
| *(void **)p = PyLong_AsVoidPtr(obj); | ||
| return !PyErr_Occurred(); |
There was a problem hiding this comment.
You can get a maybe get a speed improvement here by checking this only if *(void **)p == NULL
There was a problem hiding this comment.
I strongly doubt this would have any measurable effect on speed, so I'd rather just avoid a conditional path to keep things simple.
There was a problem hiding this comment.
PyErr_Occured calls PyThreadState_GET, which involves atomic operations. This is just like using the common pattern of ret == -1 && PyErr_Occurred() when converting integers. It may be unnecessary, but it does seem to be typical within CPython.
There was a problem hiding this comment.
It's not actually clear to me from the docs that the result would necessarily be NULL when an error occurs.
There was a problem hiding this comment.
https://docs.python.org/3/c-api/long.html#c.PyLong_AsVoidPtr
Returns NULL on error. Use PyErr_Occurred() to disambiguate.
There was a problem hiding this comment.
Ah, I was looking at 3.5 docs which didn't seem to have that line.
| { | ||
| void **val = (void **)p; | ||
| *val = PyLong_AsVoidPtr(obj); | ||
| return *val != NULL ? 1 : !PyErr_Occurred(); |
There was a problem hiding this comment.
nit: more readable as val == NULL && PyErr_Occurred() ? 0 : 1 or !(val == NULL && PyErr_Occurred())
There was a problem hiding this comment.
That's exactly why I wanted to leave it at the old version: I don't think this discussion really helps (and I think my version is more readable: if it's non-null, PyLong_AsVoidPtr succeeded and we don't need to check for an error, otherwise do check).
I could even write the whole thing as return ((void **)p = PyLong_AsVoidPtr(obj)) ? 1 : !PyErr_Occurred(); but that's just silly.
…569-on-v3.0.x Backport PR #12569 on branch v3.0.x (Don't confuse uintptr_t and Py_ssize_t.)
PR Summary
Should hopefully close #12567.
PR Checklist