8000 bpo-33954: Fix _PyUnicode_InsertThousandsGrouping() #10623
Conversation
|
@serhiy-storchaka, @methane: Would you mind to review this change? Since I moved the implementation of the function, the diff is not easy to read :-( I also had to move the FILL() macro. Changes:
I hesitate to move the function to Python/formatter_unicode.c since it's only called there. _PyUnicode_InsertThousandsGrouping() function is really ugly. It should be called twice with different parameters, and the second call requires writer to be properly initialized (prepared with the correct 'kind' and enough buffer). I don't understand why _PyUnicode_InsertThousandsGrouping() has a "min_width" parameter. It looks like an optimization specific to Python/formatter_unicode.c. This change is a bugfix and should be backported to Python 3.6 and 3.7. Currently, it's possible to crash Python, see https://bugs.python.org/issue33954 |
|
I plan to merge this PR next week. |
|
What if keep them in |
What about moving this file to Python/formatter_unicode.c? My previous attempt to split the giant Objects/unicodeobject.c: I'm also fine with leaving the code in localeutil.h, but rename it to localeutil.c and remove STRINGLIB(). |
|
I rebased my patch, squashed commited and moved back code into (a new file) localeutil.c. My PR now renames localeutil.h to localeutil.c. |
Fix str.format(), float.__format__() and complex.__format__() methods for non-ASCII decimal point when using the "n" formatter. Changes: * Rewrite _PyUnicode_InsertThousandsGrouping(): it now requires a _PyUnicodeWriter object for the buffer and a Python str for digits. * FILL() macro now also checks "0 <= start".
Hum. GitHub (Git) failed to detect that the file has been renamed, so I decide instead of keep the original filename. I modified again my change to no longer move any code. The change should be simpler to review and better keep the Git history. |
|
@serhiy-storchaka: Would you mind to review the updated PR? |
* Convert FILL() macro to static inline function * Rename the function to unicode_fill(). * Replace "i" iterator with end pointer for the end condition.
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
Fix str.format(), float.__format__() and complex.__format__() methods for non-ASCII decimal point when using the "n" formatter. Changes: * Rewrite _PyUnicode_InsertThousandsGrouping(): it now requires a _PyUnicodeWriter object for the buffer and a Python str object for digits. * Rename FILL() macro to unicode_fill(), convert it to static inline function, add "assert(0 <= start);" and rework its code. (cherry picked from commit 59423e3) Co-authored-by: Victor Stinner <vstinner@redhat.com>
|
GH-10717 is a backport of this pull request to the 3.7 branch. |
|
Sorry, @vstinner, I could not cleanly backport this to |
) Fix str.format(), float.__format__() and complex.__format__() methods for non-ASCII decimal point when using the "n" formatter. Rewrite _PyUnicode_InsertThousandsGrouping(): it now requires a _PyUnicodeWriter object for the buffer and a Python str object for digits. (cherry picked from commit 59423e3)
) (GH-10720) Fix str.format(), float.__format__() and complex.__format__() methods for non-ASCII decimal point when using the "n" formatter. Rewrite _PyUnicode_InsertThousandsGrouping(): it now requires a _PyUnicodeWriter object for the buffer and a Python str object for digits. (cherry picked from commit 59423e3) (cherry picked from commit 6f5fa1b)
For
str.format(),float.__format__()andcomplex.__format__()methodsfor non-ASCII decimal point when using the "n" formatter.
Changes:
_PyUnicode_InsertThousandsGrouping(): it now requiresa
_PyUnicodeWriterobject for the buffer and a Python strfor digits.
Objects/stringlib/localeutil.h:_PyUnicode_InsertThousandsGrouping()implementation movedto
unicodeobject.c.https://bugs.python.org/issue33954