gh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES)#125581
gh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES)#125581vstinner wants to merge 2 commits intopython:mainfrom
Conversation
Replace PyBytes_FromStringAndSize(NULL, 0) with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).
vstinner
requested review from
abalkin,
berkerpeksag,
corona10,
erlend-aasland,
pganssle and
serhiy-storchaka
as code owners
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I am not comfortable with this change.
- There is nothing wrong in using
PyBytes_FromStringAndSize(NULL, 0). It works, it worked, and it will always work. This is not inefficient or private API. PyBytes_FromStringAndSize()returns a new reference, whilePy_GetConstant()returns a borrowed reference. Even if an empty bytes object is immutable, there is a value of always using balanced incref/decref. It makes the intention clearer and the code more error-proof. When there are several exit points and some return a new reference and other return borrowed reference, you can immediately say that there is a bug. If it is not a bug, you will waste time and mental efforts in vain on analyzing the code.- This creates a tiny obstacle for future backportings.
In summary, this is a cosmetic change which makes the code more difficult to maintain without significant gain.
Py_GetConstant() returns a new reference: https://docs.python.org/dev/c-api/object.html#c.Py_GetConstant |
|
Oh, nice. That eliminates most of my objections. There is still an issue with reasoning such change. This look like change for the sake of change. |
|
I would like to get rid of
In general, I would prefer to make |
|
I failed to convince @serhiy-storchaka, so I just close my PR. |
3 participants
Replace PyBytes_FromStringAndSize(NULL, 0)
with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).