Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Don't confuse uintptr_t and Py_ssize_t.#12569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I strongly doubt this would have any measurable effect on speed, so I'd rather just avoid a conditional path to keep things simple.
eric-wieserOct 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PyErr_Occured callsPyThreadState_GET, which involves atomic operations. This is just like using the common pattern ofret == -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not actually clear to me from the docs that the result would necessarily beNULL when an error occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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(); |
eric-wieserOct 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: more readable asval == NULL && PyErr_Occurred() ? 0 : 1 or!(val == NULL && PyErr_Occurred())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 asreturn ((void **)p = PyLong_AsVoidPtr(obj)) ? 1 : !PyErr_Occurred(); but that's just silly.
…569-on-v3.0.xBackport PR#12569 on branch v3.0.x (Don't confuse uintptr_t and Py_ssize_t.)
PR Summary
Should hopefullyclose#12567.
PR Checklist