Skip to content

gh-119609, PEP 756: Add PyUnicode_Export() function - #123738

Closed
vstinner wants to merge 28 commits into
python:mainfrom
vstinner:unicode_view
Closed

gh-119609, PEP 756: Add PyUnicode_Export() function#123738
vstinner wants to merge 28 commits into
python:mainfrom
vstinner:unicode_view

Conversation

@vstinner

@vstinnervstinner commented Sep 5, 2024

Copy link
Copy Markdown
Member

Add PyUnicode_Export(), PyUnicode_GetBufferFormat() and PyUnicode_Import() functions to the limited C API.


📚 Documentation preview 📚: https://cpython-previews--123738.org.readthedocs.build/

Add PyUnicode_Export(), PyUnicode_GetBufferFormat() and
PyUnicode_Import() functions to the limited C API.
Comment threadDoc/c-api/unicode.rst Outdated
Comment threadDoc/c-api/unicode.rst Outdated
Comment threadDoc/c-api/unicode.rst Outdated
@bedevere-app

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Comment threadDoc/c-api/unicode.rst Outdated
Comment threadDoc/c-api/unicode.rst Outdated
Comment threadObjects/unicodeobject.c Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

I have made the requested changes; please review again.

@bedevere-app

Copy link
Copy Markdown

Thanks for making the requested changes!

@mdboom: please review the changes made to this pull request.

@bedevere-app
bedevere-appBot requested a review from mdboomSeptember 5, 2024 16:56
@vstinner

Copy link
Copy Markdown
MemberAuthor

@mdboom@picnixz: Thanks for your reviews. I think that I addressed most, if not all, of them :-)

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A final nitpick on my side (sorry but I only skimmed through the implementation since I don't have much energy now...).

A bit off-topic, but do we use the PRI* macros in the code base? I saw that you used the %i for formatting a uint32_t value, which usually works, but I wondered whether you prefer using the platform-dependent ones.

Comment threadObjects/unicodeobject.c Outdated
Comment threadObjects/unicodeobject.c Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

A side effect of this change is to add the __release_buffer__() method to the built-in str type.

I had to implement collections.UserString.__release_buffer__() to fix test_collections (the UserString simply raises NotImplementedError).

Comment threadDoc/c-api/unicode.rst Outdated
Comment threadObjects/unicodeobject.c
Comment threadObjects/unicodeobject.c Outdated
Comment threadObjects/unicodeobject.c Outdated
Comment threadDoc/c-api/unicode.rst Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

@serhiy-storchaka: I updated the PR to use _PyUnicode_EncodeUTF16() and _PyUnicode_EncodeUTF32(), and address your other comments.

@vstinner

Copy link
Copy Markdown
MemberAuthor

I had to remove the check "last character in a NUL character" in tests, since _PyUnicode_EncodeUTF16() and _PyUnicode_EncodeUTF32() don't write such last NUL character.

@encukou

Copy link
Copy Markdown
Member

I had to remove the check "last character in a NUL character" in tests, since _PyUnicode_EncodeUTF16() and _PyUnicode_EncodeUTF32() don't write such last NUL character.

That's a security vulnerability waiting to happen.

Since the internal buffers do have the terminating NUL, and in most cases we expose those, people will expect the NUL even if we'd explicitly document that it's not guaranteed. IMO, we need to add it.

@vstinner

Copy link
Copy Markdown
MemberAuthor

@encukou:

Since the internal buffers do have the terminating NUL, and in most cases we expose those, people will expect the NUL even if we'd explicitly document that it's not guaranteed. IMO, we need to add it.

@serhiy-storchaka: Sorry, I reverted the "Use _PyUnicode_EncodeUTF16() and _PyUnicode_EncodeUTF32()" change to get back the NUL trailing character.

@vstinner

Copy link
Copy Markdown
MemberAuthor

I'm not sure if we should guarantee that the exported buffer ends with a NUL character. I'm not sure that all Python implementations will be able to provide such guarantee in an efficient way (without having to allocate a temporary buffer for that).

@encukou

Copy link
Copy Markdown
Member

We should. As long as the API is used from C, exported strings should be NUL-terminated for safety.
Another implementation can add a function like XPyUnicode_Export_Raw; if it becomes popular CPython can adopt it as an alias of PyUnicode_Export.

@vstinner

Copy link
Copy Markdown
MemberAuthor

We should. As long as the API is used from C, exported strings should be NUL-terminated for safety.

I suggest to continue this discussion at: capi-workgroup/decisions#33 (comment)

Comment threadObjects/unicodeobject.c Outdated
@vstinnervstinner changed the title gh-119609: Add PyUnicode_Export() functiongh-119609, PEP 756: Add PyUnicode_Export() functionSep 17, 2024
@vstinner

Copy link
Copy Markdown
MemberAuthor

I withdrawn my PEP 756.

@vstinnervstinner closed this Nov 5, 2024
@vstinner
vstinner deleted the unicode_view branch November 5, 2024 15:05
Sign up for freeto 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.

5 participants

@vstinner@encukou@mdboom@serhiy-storchaka@picnixz