Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES) #125581

Closed
wants to merge 2 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 16, 2024

Replace PyBytes_FromStringAndSize(NULL, 0)
with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).

Replace PyBytes_FromStringAndSize(NULL, 0)
with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

  1. 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.
  2. PyBytes_FromStringAndSize() returns a new reference, while Py_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.
  3. 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.

@vstinner
Copy link
Member Author

PyBytes_FromStringAndSize() returns a new reference, while Py_GetConstant() returns a borrowed reference.

Py_GetConstant() returns a new reference: https://docs.python.org/dev/c-api/object.html#c.Py_GetConstant

@serhiy-storchaka
Copy link
Member

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.

@vstinner
Copy link
Member Author

I would like to get rid of PyBytes_FromStringAndSize(NULL, size) calls which create an incomplete bytes object, but PyBytes_FromStringAndSize(NULL, 0) doesn't have this issue.

Py_GetConstant(Py_CONSTANT_EMPTY_BYTES) is simpler and a little bit faster than PyBytes_FromStringAndSize(NULL, 0).

In general, I would prefer to make Py_GetConstant(Py_CONSTANT_EMPTY_BYTES) the reference function to get an empty byte string.

@vstinner
Copy link
Member Author

@vstinner
Copy link
Member Author

vstinner commented Nov 6, 2024

I failed to convince @serhiy-storchaka, so I just close my PR.

@vstinner vstinner closed this Nov 6, 2024
@vstinner vstinner deleted the empty_bytes2 branch November 6, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants