| msg277595 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-09-28 08:11 |
| PyUnicode_AsUCS4 declares to raise ValueError if buffer length is smaller than string length. But now the implementation raises SystemError. |
|
|
| msg277637 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-09-28 15:13 |
| Here is the patch. |
|
|
| msg277715 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-09-29 19:25 |
| In general the patch LGTM. I have small doubt about the name of one variable, and may be worth to add yet few tests. |
|
|
| msg277738 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-09-30 05:15 |
| v2 now adds more tests and change the problematic variable name. |
|
|
| msg277886 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-10-02 10:56 |
| v3 resolves comments on v2. |
|
|
| msg277905 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-10-02 17:39 |
| The remaining question is what should be the type of the exception. ValueError is documented exception, but SystemError is actually raised exception (and it always was raised). PyUnicode_AsUCS4() is used 6 times in 3 files in CPython code, and it should never raise this exception. PyUnicode_AsUCS4() is in public API an can be used in third party code. Seems raising this exception can be caused only by programming error in C extension. SystemError is right exception in this case. It looks to me that the code is correct and the documentation should be fixed to match the code. |
|
|
| msg277906 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-10-02 17:54 |
| Yes. Changing the implementation or the doc is still in question so in the title I just use conflicts. Scanning unicodeobject.c there seems no general rules about which to use. But actually I'm in favour of ValueError. From the description in https://docs.python.org/3/library/exceptions.html, SystemError is raised when the interpreter finds an internal error. I actually don't think this error is an interpreter internal error. But no matter what the resolution is, we can add a test for ucs4. :-) |
|
|
| msg277908 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-10-02 18:31 |
| New changeset ac838bf5499d by Serhiy Storchaka in branch '3.5': Issue #28295: Fixed the documentation and added tests for PyUnicode_AsUCS4(). https://hg.python.org/cpython/rev/ac838bf5499d New changeset 3efeafc6517a by Serhiy Storchaka in branch '3.6': Issue #28295: Fixed the documentation and added tests for PyUnicode_AsUCS4(). https://hg.python.org/cpython/rev/3efeafc6517a New changeset 94fb4c4f58dd by Serhiy Storchaka in branch 'default': Issue #28295: Fixed the documentation and added tests for PyUnicode_AsUCS4(). https://hg.python.org/cpython/rev/94fb4c4f58dd |
|
|
| msg277909 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-10-02 18:35 |
| Additional tests are always nice. Thank you for your report and your patch Xiang. |
|
|
| msg277910 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-10-02 18:50 |
| Serhiy, in 05788a9a0b88, test_invalid_sequences seems don't have to stay in CAPITest. |
|
|
| msg277911 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-10-02 19:00 |
| Good catch! Thanks Xiang. |
|
|