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-119182: Add PyUnicodeWriter C API by vstinner · Pull Request #119184 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented May 19, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: [C API] Add an efficient public PyUnicodeWriter API #119182

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request May 19, 2024
[C API] Add an efficient public PyUnicodeWriter API #119182
Closed
@vstinner vstinner added the topic-C-API label May 19, 2024
@vstinner
vstinner force-pushed the WIP_unicode_writer branch from 8de16a1 to 4d18300 Compare May 21, 2024 18:49
@vstinner vstinner mentioned this pull request May 22, 2024
Add PyUnicodeWriter API python/pythoncapi-compat#95
Merged
@vstinner
vstinner force-pushed the WIP_unicode_writer branch from 4d18300 to 308d608 Compare May 23, 2024 12:08
@vstinner vstinner mentioned this pull request May 24, 2024
Add PyUnicodeWriter API capi-workgroup/decisions#27
Closed
@vstinner
vstinner force-pushed the WIP_unicode_writer branch 2 times, most recently from 49a46ac to 14e739b Compare May 24, 2024 07:19
@vstinner
vstinner force-pushed the WIP_unicode_writer branch from 14e739b to fd7432e Compare June 5, 2024 14:54
@vstinner vstinner added the skip news label Jun 5, 2024
@vstinner
vstinner marked this pull request as ready for review June 7, 2024 19:29
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 7, 2024
vstinner added 2 commits June 7, 2024 21:29
@vstinner
pythongh-119182: Add PyUnicodeWriter C API
3c4da2e
@vstinner
PyUnicodeWriter_Create() expects a length
b12f085
Remove PyUnicodeWriter_SetOverallocate().
@vstinner
vstinner force-pushed the WIP_unicode_writer branch from fd7432e to b12f085 Compare June 7, 2024 19:33
vstinner added 2 commits June 7, 2024 21:34
@vstinner
Rename str to repr
175c239
@vstinner
Add documentation
99fa2cb
@vstinner vstinner removed the skip news label Jun 7, 2024
@vstinner

vstinner commented Jun 7, 2024

Copy link
Copy Markdown
Member Author

@erlend-aasland @serhiy-storchaka @encukou: Do you want to review this change?

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
serhiy-storchaka self-requested a review June 7, 2024 20:45
erlend-aasland
erlend-aasland reviewed Jun 9, 2024
View reviewed changes
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst
Comment on lines +1568 to +1570
*str* must be Python :class:`str` object. *start* must be greater than or
equal to 0, and less than or equal to *end*. *end* must be less than or
equal to *str* length.

@erlend-aasland erlend-aasland Jun 9, 2024

Copy link
Copy Markdown
Contributor

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.

Nit; I prefer to use SemBr for paragraphs like this.

Suggested change
*str* must be Python :class:`str` object. *start* must be greater than or
equal to 0, and less than or equal to *end*. *end* must be less than or
equal to *str* length.
*str* must be Python :class:`str` object.
*start* must be greater than or equal to 0,
and less than or equal to *end*.
*end* must be less than or equal to *str* length.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Jun 10, 2024

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.

TIL that this is called SemBr!

Breaking on comma may be too much, but I prefer to break at the sentence boundary.

Sorry, something went wrong.

Uh oh!

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

All reactions

@erlend-aasland erlend-aasland Jun 13, 2024

Copy link
Copy Markdown
Contributor

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.

Yeah. You don't need to break at comma, but I often do to minimise future diffs.

Alternative suggestion:

Suggested change
*str* must be Python :class:`str` object. *start* must be greater than or
equal to 0, and less than or equal to *end*. *end* must be less than or
equal to *str* length.
*str* must be Python :class:`str` object.
*start* must be greater than or equal to 0, and less than or equal to *end*.
*end* must be less than or equal to *str* length.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

vstinner and others added 2 commits June 10, 2024 10:02
@vstinner @erlend-aasland
Apply suggestions from code review
e3e15f0
Co-authored-by: Erlend E. Aasland <[email protected]>
@vstinner @erlend-aasland
Apply suggestions from code review
1dbb5df
Co-authored-by: Erlend E. Aasland <[email protected]>
erlend-aasland
erlend-aasland reviewed Jun 10, 2024
View reviewed changes
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

vstinner and others added 3 commits June 10, 2024 10:10
@vstinner
Update the doc
8f02e33
@vstinner
Add dots in Changelog
a1d0ab0
@vstinner @erlend-aasland
Update Objects/unicodeobject.c
e6195b7
Co-authored-by: Erlend E. Aasland <[email protected]>
@vstinner

vstinner commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

@erlend-aasland: I addressed your review. Would you mind to review the updated PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka
serhiy-storchaka reviewed Jun 10, 2024
View reviewed changes
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

@vstinner @serhiy-storchaka
Update Objects/unicodeobject.c
4865d43
Co-authored-by: Serhiy Storchaka <[email protected]>
@vstinner @serhiy-storchaka
Apply suggestions from code review
79b7c09
Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka
serhiy-storchaka reviewed Jun 10, 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.

Except using TypeError and ValueError instead of SystemError, LGTM.

But I do not insist if other core developers prefer TypeError and ValueError. I just think that SystemError is more appropriate for programming errors in using the C API.

Sorry, something went wrong.

Uh oh!

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

All reactions
serhiy-storchaka
serhiy-storchaka previously approved these changes Jun 10, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 10, 2024
malemburg
malemburg reviewed Jun 10, 2024
View reviewed changes

@malemburg malemburg 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.

A more general question in the light of the free threading patches: Are these writer APIs thread-safe ?

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

@vstinner

vstinner commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

A more general question in the light of the free threading patches: Are these writer APIs thread-safe ?

I did nothing to make them safe, so I would say that they are not thread safe. You must not share a writer between two threads. Use one writer per thread.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
Make the API atomic
db02dae
@vstinner

vstinner commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

@malemburg @serhiy-storchaka: I modified the implementation to make write operations atomic. Either the whole string is written, or "nothing is written".

In the doc, I wrote "On error, leave the writer unchanged". In practice, it's more subtle, the internal buffer can be enlarged, or its kind (UCS1, UCS2 or UCS4) can change. But from the API consumer point of view, it's as if nothing was written.

I added two unit tests to show that it's still possible to use a writer after an error.

The hack is just to save/restore writer->pos internally in the 2 functions which are not atomic: WriteUTF8() and WriteFormat().

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka
serhiy-storchaka reviewed Jun 10, 2024
View reviewed changes
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

@serhiy-storchaka
serhiy-storchaka self-requested a review June 10, 2024 16:14
@serhiy-storchaka
serhiy-storchaka dismissed their stale review June 10, 2024 16:15

I withdraw my approve because writing cannot be atomic.

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting merge labels Jun 10, 2024
serhiy-storchaka
serhiy-storchaka approved these changes Jun 10, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jun 10, 2024
@vstinner
Fix typo
10343b0
erlend-aasland
erlend-aasland approved these changes Jun 13, 2024
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

Nice; thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/unicodeobject.c
}


PyObject*

@erlend-aasland erlend-aasland Jun 13, 2024

Copy link
Copy Markdown
Contributor

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.

Nit: inconsistent with the rest of the PR.

Suggested change
PyObject*
PyObject *

Sorry, something went wrong.

Uh oh!

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

All reactions
picnixz
picnixz reviewed Jun 13, 2024
View reviewed changes
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

@vstinner
Mention PyUnicode_DecodeUTF8() in the doc
957dc05
picnixz
picnixz reviewed Jun 13, 2024
View reviewed changes

@picnixz picnixz 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 also took the liberty of exhaustively point where the 2-blank lines rule is missing (the file is huge so committing directly might help).

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/c-api/unicode.rst Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

@vstinner
Fix typo
52efb94
@vstinner

vstinner commented Jun 13, 2024

Copy link
Copy Markdown
Member Author

@picnixz: I appreciate your remarks about coding style, but IMO you're going too far. I marked your "two empty lines" suggestions as resolved. This PR is already approved, twice. It's a complex API being discussed for 1 month and I would prefer to not waste my time on coding style. However, I fixed the "an Unicode" typos, thanks :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner merged commit 5c4235c into python:main Jun 17, 2024
@vstinner
vstinner deleted the WIP_unicode_writer branch June 17, 2024 15:10
@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 17, 2024
@vstinner

vstinner commented Jun 17, 2024

Copy link
Copy Markdown
Member Author

The C API Working Group agreed to start with this minimum PyUnicodeWriter API. I merged my PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@vstinner @mrahtz
pythongh-119182: Add PyUnicodeWriter C API (python#119184)
d090a1b
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@vstinner @estyxx
pythongh-119182: Add PyUnicodeWriter C API (python#119184)
74fa012
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 approved these changes
@malemburg malemburg malemburg left review comments
@picnixz picnixz picnixz left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

topic-C-API

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.

5 participants

@vstinner @serhiy-storchaka @malemburg @picnixz @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.