-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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.
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
PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A #51579
Comments
There seems to be something wrong with the width handling code in To reproduce: replace the line return PyUnicode_FromFormat("range(%R, %R)", r->start, r->stop); in range_repr in Objects/rangeobject.c with return PyUnicode_FromFormat("range(%20R, %20R)", r->start, r->stop); On my machine (OS X 10.6), this results in a segfault when invoking Python 3.2a0 (py3k:76311M, Nov 15 2009, 19:16:40)
[GCC 4.2.1 (Apple Inc. build 5646)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> range(0, 10)
Segmentation fault Perhaps these modifiers aren't supposed to be used with a width? |
It looks like PyUnicode_FromFormatV is computing callcount incorrectly. The whole routine could use some attention, I think. |
I feel it's not proper to allow the width restrict on types %S, %R, %A. These types correspond to PyObject_Str(), PyObject_Repr, PyObject_ASCII() respectively, the results of them are usually a complete string representation of a object. If you put a width restriction on the string, it's likely that the result string is intercepted and is of no complete meaning. If you really want to put a width restriction on the result, you can use %s instead, with one or two more lines to get the corresponding char* from the object. |
Ray.Allen wrote:
I agree with that, but don't feel strongly about not allowing this If it's easy to support, why not have it ? Otherwise, I'd be +1 on |
I think under the "we're all consenting adults" doctrine that it should be allowed. If you really want that behavior, why force the char*/%s dance at each call site when it's easy enough to do it in one place? I don't think anyone supplying a width would really be surprised that it would truncate the result and possibly break round-tripping through repr. Besides, it's allowed in pure python code:
>>> '%.5r' % object()
'<obje' |
You can write "%20s" as a argument for PyUnicode_FromFormat(), but it has no effect. The width and precision modifiers are not intended to apply to string formating(%s, %S, %R, %A), only apply to integer(%d, %u, %i, %x). Though you can write "%20s", but you cannot write "%20S", "%20R" and "%20A". There can be several fixes:
Thanks to Eric's ideas. Now I'm sure I prefer the last fix. I will work out a patch for this. |
Is this really worthy to fix? |
Here is the patch, it add support to use width and precision formatters in PyUnicode_FromFormat() for type %s, %S, %R, %V, %U, %A, besides fixed two bugs, which at least I believe: 1. According to PyUnicode_FromFormat() doc: http://docs.python.org/dev/py3k/c-api/unicode.html?highlight=pyunicode_fromformat#PyUnicode_FromFormat, the "%A" should produce result of ascii(). But in the existing code, I only find code of call to ascii(object) and calculate the spaces needed for it, but not appending the ascii() output to result. Also according to my simple test, the %A doesn't work, as the following simple test function:
static PyObject *
getstr(PyObject *self, PyObject *args)
{
const char *s = "hello world";
PyObject *unicode = PyUnicode_FromString(s);
return PyUnicode_FromFormat("%A", unicode);
}
Which should return the result of calling ascii() with the object named *unicode* as its argument. The result should be a unicode object with string "hello world". But it actually return a unicode object with string "%A". This can be fixed by adding the following line:
case 'A':
in step 4.
797 if (f == '%') { Here the variable *width* cannot be correctly calculated, because the while loop will not execute, the *f currently is definitely '%'! So the width is always 0. But currently this doesn't cause error, since the following codes will ensure width >= MAX_LONG_CHARS: 834 case 'd': case 'u': case 'i': case 'x': (currently width and precision only apply to integer types:%d, %u, %i, %x, not string and object types:%s, %S, %R, %A, %U, %V ) To fix, the following line: My patch fixed these two problems. Hoping somebody could take a look at it. |
I update the patch. Hope somebody could do a review. |
1 similar comment
I update the patch. Hope somebody could do a review. |
Oooops! Sorry for re-submit the request... |
I opened other tickets related to PyUnicode_FromFormatV:
(see also bpo-10832: Add support of bytes objects in PyBytes_FromFormatV()) PyUnicode_FromFormatV() has now tests in test_unicode: issue_7330.diff should add new tests, at least to check that %20R doesn't crash. |
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); 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 += 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", 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!
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?
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?
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.
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?
Again, I guess bpo-10829 need another its own patch to fix.
Err, '%.s' causes unexpected result both with and without my patch. Maybe it's still another bug? |
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? |
Ah ok, so don't add failing tests :-)
No you don't. You can copy a substring of the input string with
You can truncate the input char* on the call to PyUnicode_DecodeUTF8: 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
"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.
If the fix (always have the same behaviour) is short, it would be nice |
Oh, yes, I got your meaning now. I'll follow this.
Oh, what if the trunked char* cannot be decoded correctly? e.g. a tow-bytes character is divided in the middle?
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. |
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�' |
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:
+ "must be a sequence, not %.200s", 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. |
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. |
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' 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': |
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.
|
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:
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 instead of if (width > (PY_SSIZE_T_MAX - ((int)*f - '0')) / 10) |
New changeset 9e0f1c3bf9b6 by Victor Stinner in branch 'default': |
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 :-) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: