Skip to content

gh-139353: Rename formatter_unicode.c to unicode_formatter.c #139723

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 8, 2025
Merged

gh-139353: Rename formatter_unicode.c to unicode_formatter.c #139723

merged 3 commits into from
Oct 8, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 7, 2025 •
edited
Loading

  • Move Python/formatter_unicode.c to Objects/unicode_formatter.c.
  • Move Objects/stringlib/localeutil.h content into unicode_formatter.c. Remove localeutil.h.
  • Move _PyUnicode_InsertThousandsGrouping() to unicode_formatter.c and mark the function as static.
  • Rename unicode_fill() to _PyUnicode_Fill() and export it in pycore_unicodeobject.h.

@bedevere-app bedevere-app bot mentioned this pull request Oct 7, 2025
Open
@vstinner
pythongh-139353: Rename formatter_unicode.c to unicode_formatter.c
9fa57c5 
* Move Python/formatter_unicode.c to Objects/unicode_formatter.c.
* Move Objects/stringlib/localeutil.h content into
  unicode_formatter.c. Remove localeutil.h.
* Move _PyUnicode_InsertThousandsGrouping() to unicode_formatter.c
  and mark the function as static.
* Rename unicode_fill() to _PyUnicode_Fill() and export it in
  pycore_unicodeobject.h.
@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

@serhiy-storchaka: Is it what you asked for?

@vstinner vstinner marked this pull request as ready for review October 7, 2025 23:21
serhiy-storchaka
serhiy-storchaka approved these changes Oct 8, 2025
View reviewed changes
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.

Well, lets go.

Please check that the formatting code that uses _PyUnicode_InsertThousandsGrouping() does not struggle due to non-inlined _PyUnicode_Fill().

Objects/unicodeobject.c Outdated
Comment on lines 424 to 426
void
_PyUnicode_Fill(int kind, void *data, Py_UCS4 value,
Py_ssize_t start, Py_ssize_t length)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that it will be inlined? Maybe keep static inline unicode_fill() and add a wrapper _PyUnicode_Fill()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe keep static inline unicode_fill() and add a wrapper _PyUnicode_Fill()?

Ok, I implemented _PyUnicode_Fill() as a static inline function. So the compiler can decide to inline it or not.

@vstinner
Copy link
Member Author

vstinner commented Oct 8, 2025

Please check that the formatting code that uses _PyUnicode_InsertThousandsGrouping() does not struggle due to non-inlined _PyUnicode_Fill().

I checked the Python 3.14 executable on Fedora which is built with PGO+LTO. I looked at _PyUnicode_InsertThousandsGrouping() x86-64 assembly code in gdb. I found:

...
   0x00007ffff7abe483 <+339>:	call   0x7ffff7abe700 <unicode_fill.lto_priv.0>
...
   0x00007ffff7abe665 <+821>:	call   0x7ffff7abe700 <unicode_fill.lto_priv.0>
...

So in the Fedora package at least, unicode_fill() is not inlined in _PyUnicode_InsertThousandsGrouping(). Inlining is not always a good idea, it can be bad for the L1-instruction cache and puts more pressure on register allocation. I trust the compiler to decide if it's a good idea to inline or not.

@vstinner
Implement _PyUnicode_Fill() as static inline function
a9e27d9 
Move MAX_UNICODE to pycore_unicodeobject.h as _Py_MAX_UNICODE.
serhiy-storchaka
serhiy-storchaka approved these changes Oct 8, 2025
View reviewed changes
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.

Oh, I did not suggest to make it inline. I only asked if this makes noticeable difference. Locale specific formatting is complex, so I think it would not, even if it was inlined previously, but we should check.

Anyway, LGTM. 👍

@vstinner vstinner merged commit 3d3f126 into python:main Oct 8, 2025
43 checks passed
@vstinner vstinner deleted the unicode_formatter branch October 8, 2025 12:56
@vstinner vstinner mentioned this pull request Oct 8, 2025
Merged
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

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

@emmatyping emmatyping Awaiting requested review from emmatyping emmatyping is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vstinner @serhiy-storchaka