Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES)#125581
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
Replace PyBytes_FromStringAndSize(NULL, 0)with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).
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.
Please wait.
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 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. |
Uh oh!
There was an error while loading.Please reload this page.
Replace PyBytes_FromStringAndSize(NULL, 0)
with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).