gh-98836: Extend PyUnicode_FromFormat() by serhiy-storchaka · Pull Request #98838 · python/cpython

serhiy-storchaka · 2022-10-29T07:19:48Z

Support for conversion specifiers o (octal) and X (uppercase hexadecimal).
Support for length modifiers j (intmax_t) and t (ptrdiff_t).
Length modifiers are now applied to all integer conversions.
Support for wchar_t C strings (%ls and %lV).
Support for variable width and precision (*).
Support for flag - (left alignment).

Issue: Extend PyUnicode_FromFormat() #98836

* Support for conversion specifiers o (octal) and X (uppercase hexadecimal). * Support for length modifiers j (intmax_t) and t (ptrdiff_t). * Length modifiers are now applied to all integer conversions. * Support for wchar_t C strings (%ls and %lV). * Support for variable width and precision (*). * Support for flag - (left alignment).

vstinner · 2022-11-04T18:32:25Z

Doc/whatsnew/3.12.rst

+  :c:func:`PyUnicode_FromFormatV` now sets a :exc:`SystemError`.
+  In previous versions it caused all the rest of the format string to be
+  copied as-is to the result string, and any extra arguments discarded.
+  (Contributed by Serhiy Storchaka in :gh:`95781`.)


It seems like this paragraph was duplicated by mistake?

vstinner · 2022-11-04T18:34:48Z

Lib/test/test_unicode.py

            c_int, c_long, c_longlong, c_ssize_t,
-            c_uint, c_ulong, c_ulonglong, c_size_t, c_void_p)
+            c_uint, c_ulong, c_ulonglong, c_size_t, c_void_p,
+            sizeof, c_wchar, c_wchar_p)


Can you please a comment somewhere to list formats which are not tested and the reason why?

I see at least %j and %t which are not tested. Please mention that _testcapi.test_string_from_format() has a wider coverage of all formats, and test these formats.

vstinner · 2022-11-04T18:36:53Z

Lib/test/test_unicode.py

+        check_format('   0000123', b'%10.7i', c_int(123))
+        check_format('0000000123', b'%010.7i', c_int(123))
+        check_format('0000123   ', b'%-10.7i', c_int(123))
+        check_format('0000123   ', b'%-010.7i', c_int(123))


Would it be possible to generate these tests? The code is similar for the 6 groups of tests.

It is not so similar. It is different for signed and unsigned types, but I'll try to generalize it.

I just suggested to generate these tests. It's ok to leave them as they are if it's too complicated to generate them.

vstinner · 2022-11-04T18:37:45Z

Lib/test/test_unicode.py

+                     b'%V', None, b'xyz')
+
+        # test %ls
+        check_format('abc', b'%ls', c_wchar_p('abc'))


You can please add one non-ASCII character in wchar tests, %ls and %lV? In addition to tests truncating to 2 wchar_t.

What do you mean? Isn't the following line enough?

I'm thinking at adding a test with non-ASCII characters which fit into UCS-2 (16-bit wchar_t) characters. Something like:

check_format('a\xe9\u20ac', b'%ls', c_wchar_p('a\xe9\u20ac'))

Some bugs are only triggered depending on the maximum code point, and wchar_t code can take different code path depending if there is a surrogate character or not.

vstinner · 2022-11-04T18:39:04Z

Lib/test/test_unicode.py

@@ -2880,10 +2989,11 @@ def check_format(expected, format, *args):
        # check for crashes


this comment is misleading. Python doesn't crash but reject invalid format string with a SyntaxError. Would you mind to rephrase the comment? Like: # test invalid format strings?

Done. Restored the part of the older comment.

vstinner · 2022-11-04T18:47:25Z

Doc/c-api/unicode.rst

-   |                   |                     | :c:func:`PyObject_Repr`.         |
-   +-------------------+---------------------+----------------------------------+
+   ASCII-encoded string.
+


Can you please add something like [[fill]align][sign][z][#][0][width][grouping_option][.precision][type]? Example taken from: https://docs.python.org/dev/library/string.html#format-specification-mini-language

The printf format is complex, and it's not obvious in which order each part should be written. I understand that it's something like:

%[conversion flags][minimum width][.precision][length modifier][conversion type]

Or if you prefer a shorter version:

%[conversion][width][.precision][modifier][conversion]

I copied the following paragraphs from the description of the print-like formatting in Python: https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting.

I think that it also can benefit from from similar changes, but let leave it to other issue.

vstinner

LGTM.

vstinner · 2022-11-07T21:58:46Z

Lib/test/test_unicode.py

+        check_format('   0000123', b'%10.7i', c_int(123))
+        check_format('0000000123', b'%010.7i', c_int(123))
+        check_format('0000123   ', b'%-10.7i', c_int(123))
+        check_format('0000123   ', b'%-010.7i', c_int(123))


I just suggested to generate these tests. It's ok to leave them as they are if it's too complicated to generate them.

vstinner · 2022-11-07T21:59:29Z

Lib/test/test_unicode.py

+        # Length modifiers "j" and "t" are not tested here because ctypes does
+        # not expose types for intmax_t and ptrdiff_t.
+        # _testcapi.test_string_from_format() has a wider coverage of all
+        # formats.


Thank you, this comment is useful to me at least :-)

vstinner · 2022-11-07T22:02:57Z

Lib/test/test_unicode.py

+                     b'%V', None, b'xyz')
+
+        # test %ls
+        check_format('abc', b'%ls', c_wchar_p('abc'))


I'm thinking at adding a test with non-ASCII characters which fit into UCS-2 (16-bit wchar_t) characters. Something like:

check_format('a\xe9\u20ac', b'%ls', c_wchar_p('a\xe9\u20ac'))

Some bugs are only triggered depending on the maximum code point, and wchar_t code can take different code path depending if there is a surrogate character or not.

vstinner · 2022-11-07T22:03:40Z

Feel free to ignore my suggestions, the PR LGTM.

vstinner

LGTM.

bedevere-bot added the awaiting core review label Oct 29, 2022

serhiy-storchaka added 5 commits October 29, 2022 17:48

Fix docs formatting.

4fdf596

Add tests for integer conversions.

13d4c97

Merge branch 'main' into capi-unicode-fromformat

e69d8b0

Add more tests.

3bd9d3e

Finish documentation.

1e6094b

bedevere-bot mentioned this pull request Nov 3, 2022

Extend PyUnicode_FromFormat() #98836

Closed

serhiy-storchaka marked this pull request as ready for review November 3, 2022 15:23

serhiy-storchaka requested review from lysnikolaou and pablogsal as code owners November 3, 2022 15:23

serhiy-storchaka requested review from vstinner and removed request for lysnikolaou and pablogsal November 3, 2022 18:02

vstinner reviewed Nov 4, 2022

View reviewed changes

serhiy-storchaka added 5 commits November 6, 2022 19:35

Merge branch 'main' into capi-unicode-fromformat

8942ca3

Remove duplicated paragraph.

8481bda

Address review comments.

8885850

Fix syntax error in a comment.

9ffdb86

Fix and refactor tests.

9ca3764

vstinner approved these changes Nov 7, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Nov 7, 2022

serhiy-storchaka added 2 commits November 20, 2022 11:38

Merge branch 'main' into capi-unicode-fromformat

79a523c

Add more tests for %ls and %lV.

91d8393

vstinner approved these changes Nov 21, 2022

View reviewed changes

serhiy-storchaka added 2 commits May 21, 2023 23:11

Merge branch 'main' into capi-unicode-fromformat

9f2f5f2

Try to silence Sphinx warnings.

b86d92d

serhiy-storchaka merged commit f3466bc into python:main May 21, 2023

bedevere-bot removed the awaiting merge label May 21, 2023

serhiy-storchaka deleted the capi-unicode-fromformat branch May 21, 2023 21:32

		@@ -2880,10 +2989,11 @@ def check_format(expected, format, *args):
		# check for crashes

Uh oh!

Conversation

serhiy-storchaka commented Oct 29, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 7, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-storchaka commented Oct 29, 2022 •

edited by bedevere-bot

Loading