PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A

@malemburg

BPO	7330
Nosy	@malemburg, @mdickinson, @vstinner, @ericvsmith, @ezio-melotti, @lekma, @akheron, @serhiy-storchaka
Files	issue_7330.diff unicode_fromformat_precision.patch unicode_fromformat_precision-2.patch unicode_fromformat_precision-3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-05-06.21:36:01.271>
created_at = <Date 2009-11-15.19:42:32.127>
labels = ['type-feature', 'expert-unicode']
title = 'PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A'
updated_at = <Date 2013-05-06.21:36:01.270>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2013-05-06.21:36:01.270>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-05-06.21:36:01.271>
closer = 'vstinner'
components = ['Unicode']
creation = <Date 2009-11-15.19:42:32.127>
creator = 'mark.dickinson'
dependencies = []
files = ['21324', '27465', '27479', '30144']
hgrepos = []
issue_num = 7330
keywords = ['patch', 'needs review']
message_count = 42.0
messages = ['95306', '95310', '111802', '111808', '111820', '111894', '112041', '112298', '117995', '117996', '117997', '127690', '128296', '128359', '128381', '128725', '128773', '128776', '128785', '128786', '128790', '128933', '129942', '130258', '131649', '131668', '131710', '131964', '131965', '131968', '132057', '144626', '147861', '147966', '172258', '172262', '172343', '177206', '188476', '188477', '188596', '188597']
nosy_count = 12.0
nosy_names = ['lemburg', 'mark.dickinson', 'vstinner', 'eric.smith', 'ron_adam', 'ezio.melotti', 'lekma', 'ysj.ray', 'python-dev', 'petri.lehtinen', 'serhiy.storchaka', 'Sean.Ochoa']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue7330'
versions = ['Python 3.4']

Thanks haypo!

Here is the updated patch, it add the tests about width modifiers and precision modifiers of %S, %R, %A. Besides I don't know how to add tests of %s, since when calling through ctypes, I could not get correct result value as python object from PyUnicode_FromFormat() with '%s' in format string as argument.

Here's the complete patch, added unittest for width modifier and precision modifier for '%s' formatter of PyUnicode_FromFormat() function.

It looks like your patch fixes bpo-10829: you should add tests for that, you can just reuse the tests of my patch (attached to bpo-10829).

---

unicode_format() looks suboptimal.

+ memset(buffer, ' ', width);
+ width_unicode = PyUnicode_FromStringAndSize(buffer, width);

You should avoid this byte string (buffer) and use memset() on the Unicode string directly. Something like:

Py_UNICODE *u;
Py_ssize_t i;
width_unicode = PyUnicode_FromUnicode(NULL, width);
u = PyUnicode_AS_UNICODE(width_unicode);
for(i=0; i < width; i++) {
  *u = (Py_UNICODE)' ';
  u++;
}

You should also avoid the creation of a temporary unicode object (it can be slow if precision is large) using PySequence_GetSlice(). Py_UNICODE_COPY() does already truncate the string because you can pass an arbitrary length.

---

I don't like "unicode_format" function name: it sounds like "str.format()" in Python. A suggestion: "unicode_format_align"

---

With your patch, "%.200s" truncates the input string to 200 *characters*, but I think that it should truncate to 200 *bytes*, as printf does.

---

           n += PyUnicode_GET_SIZE(str);

+ n += width > PyUnicode_GET_SIZE(str) ? width : PyUnicode_GET_SIZE(str);

I don't like this change because I hate having to compute manually strings length. It should that it would be easier if you format directly strings with width and precision at step 3, instead of doing it at step 4: so you can just read the length of the formatted string, and it avoids having to handle width/precision in two steps (which may be inconsistent :-/).

---

Your patch implements %.100s (and %.100U): we might decide what to do with bpo-10833 before commiting your patch.

---

In my opinion, the patch is a little bit too big. We may first commit the fix on the code parsing the width and precision: fix bpo-10829?

---

Can you add tests for "%.s"? I would like to know if "%.s" is different than "%s" :-)

---

                            "must be a sequence, not %200s",

+ "must be a sequence, not %.200s",

Hum, I think that they are many other places where such fix should be done. Nobody noticed this typo before because %.200s nor %200s were implemented (bpo-10833).

---

Finally, do you really need to implement %200s, %2.5s and %.100s? I don't know, but I would be ok to commit the patch if you fix it for all of my remarks :-)

Thanks hyapo!

It looks like your patch fixes bpo-10829: you should add tests for that, you can just reuse the tests of my patch (attached to bpo-10829).

Sorry, but I think my patch doesn't fix bpo-10829. It seems link another issue. And by applying my patch and add tests from bpo-10829's patch, the tests cannot passed. Or did I missed something?

You should also avoid the creation of a temporary unicode object (it can be slow if precision is large) using PySequence_GetSlice(). Py_UNICODE_COPY() does already truncate the string because you can pass an arbitrary length.

In order to use Py_UNICODE_COPY, I have to create a unicode object with required length first. I feel this have the same cost as using PySequence_GetSlice(). If I understand correctly?

With your patch, "%.200s" truncates the input string to 200 *characters*, but I think that it should truncate to 200 *bytes*, as printf does.

Sorry, I don't understand. The result of PyUnicode_FromFormatV() is a unicode object. Then how to truncate to 200 *bytes*? I think the %s formatter just indicate that the argument is c-style chars, the result is always unicode string, and the width and precision formatters are to applied after converting c-style chars to string.

I don't like this change because I hate having to compute manually strings length. It should that it would be easier if you format directly strings with width and precision at step 3, instead of doing it at step 4: so you can just read the length of the formatted string, and it avoids having to handle width/precision in two steps (which may be inconsistent :-/).

Do you mean combine step 3 and step 4 together? Currently step 3 is just to compute the biggest width value and step 4 is to compute exact width and do the real format work. Only by doing real format we can get the exact width of a string. So I have to compute each width twice in both step 3 and step 4. Is combining the two steps in to one a good idea?

In my opinion, the patch is a little bit too big. We may first commit the fix on the code parsing the width and precision: fix bpo-10829?

Again, I guess bpo-10829 need another its own patch to fix.

Can you add tests for "%.s"? I would like to know if "%.s" is different than "%s" :-)

Err, '%.s' causes unexpected result both with and without my patch. Maybe it's still another bug?

Do you mean combine step 3 and step 4 together? Currently step 3 is just to compute the biggest width value and step 4 is to compute exact width and do the real format work. Only by doing real format we can get the exact width of a string. So I have to compute each width twice in both step 3 and step 4. Is combining the two steps in to one a good idea?

Sorry, Here I mean:

Do you mean combine step 3 and step 4 together? Currently step 3 is just to compute the biggest width value and step 4 is to compute exact width and do the convert work(by calling PyObject_Str()/PyObject_Repr()/PyObject_ASCII()/PyUnicode_DecodeUTF8() for %S/%R/%A/%s). Only by doing convert we can get the exact width of a string. So I have to compute each width twice in both step 3 and step 4. Is combining the two steps in to one a good idea?

> It looks like your patch fixes bpo-10829: you should add tests for that, you can just reuse the tests of my patch (attached to bpo-10829).

Sorry, but I think my patch doesn't fix bpo-10829.

Ah ok, so don't add failing tests :-)

> You should also avoid the creation of a temporary unicode object (it can be slow if precision is large) using PySequence_GetSlice(). Py_UNICODE_COPY() does already truncate the string because you can pass an arbitrary length.

In order to use Py_UNICODE_COPY, I have to create a unicode object with required length first.

No you don't. You can copy a substring of the input string with
Py_UNICODE_COPY: just pass a smaller length.

> With your patch, "%.200s" truncates the input string to 200 *characters*, but I think that it should truncate to 200 *bytes*, as printf does.

Sorry, I don't understand. The result of PyUnicode_FromFormatV() is a unicode object. Then how to truncate to 200 *bytes*?

You can truncate the input char* on the call to PyUnicode_DecodeUTF8:
pass a size smaller than strlen(s).

case 's':
{
    /* UTF-8 */
    const char *s = va_arg(count, const char*);
    PyObject *str = PyUnicode_DecodeUTF8(s, strlen(s), "replace");
    if (!str)
        goto fail;
    n += PyUnicode_GET_SIZE(str);
    /* Remember the str and switch to the next slot */
    *callresult++ = str;
    break;
}

I don't know if we should truncate to a number of bytes, or a number of
characters.

> I don't like this change because I hate having to compute manually strings length. It should that it would be easier if you format directly strings with width and precision at step 3, instead of doing it at step 4: so you can just read the length of the formatted string, and it avoids having to handle width/precision in two steps (which may be inconsistent :-/).

Do you mean combine step 3 and step 4 together? Currently step 3 is just to compute the biggest width value and step 4 is to compute exact width and do the real format work. Only by doing real format we can get the exact width of a string. So I have to compute each width twice in both step 3 and step 4. Is combining the two steps in to one a good idea?

"Do you mean combine step 3 and step 4 together?"

Yes, but I am no more sure that it is the right thing to do.

> Can you add tests for "%.s"? I would like to know if "%.s" is different than "%s" :-)

Err, '%.s' causes unexpected result both with and without my patch. Maybe it's still another bug?

If the fix (always have the same behaviour) is short, it would be nice
to include it in your patch.

No you don't. You can copy a substring of the input string with
Py_UNICODE_COPY: just pass a smaller length.

Oh, yes, I got your meaning now. I'll follow this.

You can truncate the input char* on the call to PyUnicode_DecodeUTF8:

Oh, what if the trunked char* cannot be decoded correctly? e.g. a tow-bytes character is divided in the middle?

Yes, but I am no more sure that it is the right thing to do.

If I understand correctly(my English ability is limited), your suggestion is to combine, right? I'm afraid that combine may bring us too complicated code to write. The currently 4 steps just divide the process into smaller and simpler pieces. I'm not sure.

Oh, what if the trunked char* cannot be decoded correctly?
e.g. a tow-bytes character is divided in the middle?

Yes, but PyUnicode_FromFormatV() uses UTF-8 decoder with replace error handler, and so the incomplete byte sequence will be replaced by � (it doesn't fail with an error). Example:

>>> "abc€".encode("utf-8")[:-1].decode("utf-8", "replace")
'abc�'

Can you add tests for "%.s"? I would like to know if "%.s" is different than "%s" :-)

Oh sorry~~ I made an mistake. There is no bug here. I have attached tests that show that '%.s' is the same as '%s'.

Here is the updated patch:
1, changed the function name unicode_format() to
1, remove
"""

                            "must be a sequence, not %200s",

+ "must be a sequence, not %.200s",
"""
in Python/ceval.c

2, Removing using PySequence_GetSlice() in unicode_format_align() and do a refactor to optimize the process.

3, Add tests for '%.s' and '%s', as haypo wanted.

This is obviously not the final patch just convenient for other to do a review. Something more need to be discussed.

> > With your patch, "%.200s" truncates the input string to 200 *characters*, but I think that it should truncate to 200 *bytes*, as printf does.
>
> Sorry, I don't understand. The result of PyUnicode_FromFormatV() is a unicode object. Then how to truncate to 200 *bytes*?

You can truncate the input char* on the call to PyUnicode_DecodeUTF8:
pass a size smaller than strlen(s).

Now I wonder how should we treat precision formatters of '%s'. First of all, the PyUnicode_FromFormat() should behave like C printf(). In C printf(), the precision formatter of %s is to specify a maximum width of the displayed result. If final result is longer than that value, it must be truncated. That means the precision is applied on the final result. While python's PyUnicode_FromFormat() is to produce unicode strings, so the width and precision formatter should be applied on the final unicode string result. And the format stage is split into two ones, one is converting each paramater to an unicode string, another one is to put the width and precision formatters on them. So I wonder if we should apply the precision formatter on the converting stage, that is, to PyUnicode_DecodeUTF8(). So in my opinion precision should not be applied to input chars, but output unicodes.

I hope I didn't misunderstand something.

So haypo, what's your opinion.

Here is the updated patch:

1, Work with function parse_format_flags() which is introduced in bpo-10829, and the patch is simpler and more clear than before.
2, Change parse_format_flags() to set precision value to -1 in the case of '%s' in order to differ with '%.0s'
3, Move call of unicode_format_align() in step 3 in order to avoid many codes like "n += width > PyUnicode_GET_SIZE(str) ? width : PyUnicode_GET_SIZE(str);", (following haypo's comments)

I noticed that after apply my last patch and running full unittest cases, some weird errors which I don't know the reasons occurred, for example:

AttributeError: 'dict' object has no attribute 'get'
and
AttributeError: 'Queue' object has no attribute 'get'

I didn't look deep into it. But I found after I optimist my patch, these errors disappeared: I removed the "unicode_format_align()" function in previous patch, directly add needed spaces and copy part of unicode got from parameters according to width and precision formatters in step 4(using Py_UNICODE_FILL() and Py_UNICODE_COPY()) . This avoid create temporary unicode objects using unicode_format_align() in step 3. And also the patch becomes simpler.

So this patch is intended to replace of the previous. And if I have more time, I will try to find the reasons of the weird errors.

Ray Allen: Your patch doesn't touch the documentation. At least, you should mention (using .. versionchanged:: 3.3) that PyUnicode_FromFormat() does now support width and precision. It is important to specify the unit of the sizes: number of bytes or number of characters? Because many developer may refer to printf() which counts in bytes (especially for %s). PyUnicode_FromFormat() is more close to wprintf(), but I don't know if wprintf() uses bytes or characters for width and precision with the %s and %ls formats.

I plan to fix bpo-10833 by replacing %.100s by %s is most (or all) error messages, and then commit your patch.

Ooops! I found my last submitted patch is a wrong one.

Here is the updated patch add doc entries about the changes. The test cases which assert error messages generated by PyUnicode_FromFormat() with "%.200s" formatters equality would failed due to this patch. Hope you don't miss any of them.

New changeset d3ae3fe3eb97 by Victor Stinner in branch 'default':
Issue bpo-7330, bpo-10833: Replace %100s by %.100s and %200s by %.200s
http://hg.python.org/cpython/rev/d3ae3fe3eb97

By the way, as my simple tests, wprintf() with "%ls" does apply the width and precision formatters on units of characters.

There are 4 patches "bpo-7030" attached to this issue. Some of them have a version number in their name, some doesn't. You did the same on other issues. It is more easy to follow a patch if it has a version number, for example: issue_7330.diff, issue_7330-2.diff, issue_7330-3.diff, issue_7330-4.diff, ... And I suppose that you can remove all old patches, except if they are alternative implementations or contain something special.

Sorry for having done that! I will remove old patches and leave a cleaner view.

I closed bpo-10833 as invalid, because it is a regression of Python 3. PyErr_String() uses PyString_FromFormatV() in Python 2, which supports precision for %s, whereas it uses PyUnicode_FromFormatV() in Python 3, which never supported precision for %s.

Hum, the issue is still open, I will try to review it.

Issue bpo-13428 has been marked as a duplicate of this issue.

Hi!

I'd like to have this committed to be able to fix bpo-13349. So here's a review.

In Doc/c-api/unicode.rst, the two "versionchanged:: 3.3" directives can be merged
In tests, I'd use 'abcde' rather than 'xxxxx' to make sure that correct characters are copied to the output (hope you understand what I mean)
No test checks that width and precision work on characters rather than bytes
The changes to unicodeobject.c don't apply on top of current default branch.

I rewrote PyUnicode_FromFormatV() to use a single step instead of four: see issue bpo-16147. So it's now simpler to fix this issue. Here is a new patch to implement width and precision modifiers for %s, %A, %R, %S and %U formats.

I read again this old issue. I still think that it would be better to truncate to a number of *bytes* for "%s" format (and %V format when the first argument is NULL) to mimic printf(). The "replace" error handler of the UTF-8 decoder handles truncated string correctly. So I should update my patch.

Updated patch: precision for "%s" and "%V" (if the first PyObject* argument is NULL) formats is now a number of bytes, rather than a number of characters. width is still always a number of character.

The reason is that "%.100s" can be used for avoid a crash if the argument is not terminated by a null character (see issue bpo-10833).

I found one bug and add some nitpicks and optimization suggestion on Rietveld.

New version of my patch taking Serhiy's remarks into account:

add a check_format() function to cleanup unit tests
only call _PyUnicodeWriter_Prepare() once per formatted argument: compute the length and maximum character. Be more optimistic about sprintf() for integer and pointer: expect that the maximum character is 127 or less
uniformize code parsing width and precision
factorize code for '%s' and '%V'

Note: remove also _PyUnicode_WriteSubstring() from the patch, it was already added.

I didn't add the following optimization (proposed by Serhiy in his review) because I'm not convinced that it's faster, and it's unrelated to this issue:

if (width > (PY_SSIZE_T_MAX - 9) / 10
&& width > (PY_SSIZE_T_MAX - ((int)*f - '0')) / 10)
{ ... }

instead of

if (width > (PY_SSIZE_T_MAX - ((int)*f - '0')) / 10)
{ ... }

New changeset 9e0f1c3bf9b6 by Victor Stinner in branch 'default':
Issue bpo-7330: Implement width and precision (ex: "%5.3s") for the format string
http://hg.python.org/cpython/rev/9e0f1c3bf9b6

Finally, I closed this issue. Sorry for the long delay, but many other PyUnicode_FromFormat() issues had to be discussed/fixed before this one can be fixed. It was also much easier to fix this issue since my refactoring of PyUnicode_FromFormat() to only parse the format string once (thanks to the _PyUnicodeWriter API) instead of having 4 steps.

Thanks to Ysj Ray, thanks to reviewers.

This is one of the oldest issue that I had to fix :-)

Navigation Menu

Repository navigation

PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A #51579

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A #51579

Description

Activity

ysjray commented on Feb 10, 2011

ysjray commented on Feb 11, 2011

vstinner commented on Feb 11, 2011

ysjray commented on Feb 17, 2011

ysjray commented on Feb 18, 2011

vstinner commented on Feb 18, 2011

ysjray commented on Feb 18, 2011

vstinner commented on Feb 18, 2011

ysjray commented on Feb 18, 2011

ysjray commented on Feb 21, 2011

ysjray commented on Mar 3, 2011

ysjray commented on Mar 7, 2011

vstinner commented on Mar 21, 2011

ysjray commented on Mar 21, 2011

python-dev commented on Mar 22, 2011

ysjray commented on Mar 24, 2011

vstinner commented on Mar 24, 2011

ysjray commented on Mar 24, 2011

vstinner commented on Mar 25, 2011

vstinner commented on Sep 29, 2011

vstinner commented on Nov 18, 2011

akheron commented on Nov 19, 2011

vstinner commented on Oct 6, 2012

vstinner commented on Oct 6, 2012

vstinner commented on Oct 7, 2012

serhiy-storchaka commented on Dec 9, 2012

vstinner commented on May 5, 2013

vstinner commented on May 5, 2013

python-dev commented on May 6, 2013

vstinner commented on May 6, 2013

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions