classification
Title: PyUnicode_AsUCS4 doc and impl conflict on exception
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: python-dev, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-09-28 08:11 by xiang.zhang, last changed 2016-10-02 19:00 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
PyUnicode_AsUCS4.patch xiang.zhang, 2016-09-28 15:13 review
PyUnicode_AsUCS4_v2.patch xiang.zhang, 2016-09-30 05:15 review
PyUnicode_AsUCS4_v3.patch xiang.zhang, 2016-10-02 10:56 review
Messages (11)
msg277595 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) 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) * (Python committer) Date: 2016-09-28 15:13
Here is the patch.
msg277715 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) Date: 2016-09-30 05:15
v2 now adds more tests and change the problematic variable name.
msg277886 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-10-02 10:56
v3 resolves comments on v2.
msg277905 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2016-10-02 19:00
Good catch! Thanks Xiang.
History
Date User Action Args
2016-10-02 19:00:59serhiy.storchakasetmessages: + msg277911
2016-10-02 18:50:39xiang.zhangsetmessages: + msg277910
2016-10-02 18:35:41serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2016-10-02 18:35:19serhiy.storchakasetmessages: + msg277909
2016-10-02 18:31:24python-devsetnosy: + python-dev
messages: + msg277908
2016-10-02 17:54:06xiang.zhangsetmessages: + msg277906
2016-10-02 17:39:02serhiy.storchakasetmessages: + msg277905
2016-10-02 10:56:17xiang.zhangsetfiles: + PyUnicode_AsUCS4_v3.patch

messages: + msg277886
2016-09-30 05:16:00xiang.zhangsetfiles: + PyUnicode_AsUCS4_v2.patch

messages: + msg277738
2016-09-29 19:25:05serhiy.storchakasetmessages: + msg277715
2016-09-28 15:13:41xiang.zhangsetfiles: + PyUnicode_AsUCS4.patch
keywords: + patch
messages: + msg277637

stage: patch review
2016-09-28 08:11:06xiang.zhangcreate