fix: check return code in unpack_callback_int64#665
Conversation
There was a problem hiding this comment.
Should this be PyLong_FromUnsignedLong((unsigned long)d)?
There was a problem hiding this comment.
Both are correct. Since we checked d <= LONG_MAX, (long)d is guaranteed to fit in a signed long.
But p = PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)d) without d > LONG_MAX check would be better.
There was a problem hiding this comment.
OK, agreed.
But if both are correct, one could use the less eyebrows-raising "unsigned" call and cast?
There was a problem hiding this comment.
Do you want me to change this in the PR?
There was a problem hiding this comment.
@methane decides.
It is somehow unrelated to the PR topic, I just stumbled over it while looking at the code.
There was a problem hiding this comment.
But if both are correct, one could use the less eyebrows-raising "unsigned" call and cast?
There is no PyLong_FromUnsignedLong(). PyLong_FromLong requires (unsigned long) cast to suppress implicit cast warning.
Do you want me to change this in the PR?
Please remove if (d > LONG_MAX) { and use PyLong_FromUnsignedLongLong always.
There was a problem hiding this comment.
There is no PyLong_FromUnsignedLong()
There is a call to that in line 333.
There was a problem hiding this comment.
You are right. But using only PyLong_FromUnsignedLongLong is much simpler.
There was a problem hiding this comment.
I removed the call and now it's always PyLong_FromUnsignedLongLong!
4b0097e to
5142b3d
Compare
| } else { | ||
| p = PyLong_FromLong((long)d); | ||
| } | ||
| PyObject *p = PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)d); |
There was a problem hiding this comment.
Interesting, but it seems correct.
(I am not really familiar with the C api.)
| @@ -109,6 +104,8 @@ static inline int unpack_callback_int64(unpack_user* u, int64_t d, msgpack_unpac | |||
| } else { | |||
| p = PyLong_FromLong((long)d); | |||
| } | |||
There was a problem hiding this comment.
but if that other change is correct, then this one can also be simplified in the same way, right?
Similar to #665, just a return value check to propagate the error in case one happens (instead of silently ignoring it). Note that as opposed to the previous lines, we don't need to `PyErr_SetString` since `unpack_callback_uint32` calls `PyLong_FromSize_t` which itself should set whatever Python error is relevant; we just need to make it clear to the caller that an error occurred.
Refactor unpack_callback_int64 to simplify PyObject creation.
This simply adds a null pointer check after calling
PyLong_FromLongLong/PyLong_FromLonglike other similar functions do it (example here).