Skip to content

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 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