GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES) by vstinner · Pull Request #125581 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Oct 16, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: C API: Implement singletons as functions calls in the stable ABI 3.13 #115754

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
pythongh-115754: Use Py_GetConstant(Py_CONSTANT_EMPTY_BYTES)
e65dcc8 
Replace PyBytes_FromStringAndSize(NULL, 0)
with Py_GetConstant(Py_CONSTANT_EMPTY_BYTES).
@vstinner vstinner added the skip news label Oct 16, 2024
@vstinner
vstinner requested review from abalkin, berkerpeksag, corona10, erlend-aasland, pganssle and serhiy-storchaka as code owners October 16, 2024 12:04
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 16, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Oct 16, 2024
C API: Implement singletons as functions calls in the stable ABI 3.13 #115754
Closed
erlend-aasland
erlend-aasland approved these changes Oct 16, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 16, 2024
serhiy-storchaka
serhiy-storchaka reviewed Oct 16, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka
serhiy-storchaka reviewed Oct 16, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

  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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Oct 16, 2024

Copy link
Copy Markdown
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
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Oct 16, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Oct 25, 2024

Copy link
Copy Markdown
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.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

See also https://discuss.python.org/t/what-do-you-think-of-deprecating-c-apis-to-modify-immutable-strings/67057
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
Merge branch 'main' into empty_bytes2
cb14681
@vstinner

vstinner commented Nov 6, 2024

Copy link
Copy Markdown
Member Author

I failed to convince @serhiy-storchaka, so I just close my PR.
erlend-aasland reacted with confused emoji
All reactions
  • 😕 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@abalkin abalkin Awaiting requested review from abalkin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@vstinner @serhiy-storchaka @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.