bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples by orenmn · Pull Request #3119 · python/cpython
Conversation
according to http://bugs.python.org/issue28261, fix some error messages.
without this patch, each of the following calls would have produced a wrong error message (at least on Windows 10):
ctypes.CFUNCTYPE(None)(())_io.IncrementalNewlineDecoder(42, 42).setstate(())audioop.ratecv(b'a', 1, 1, 42, 42, (42, ((),)))audioop.lin2adpcm(b'a', 1, ())audioop.adpcm2lin(b'a', 1, ())_overlapped.Overlapped().ConnectEx(42, ())(also,_overlapped.Overlapped().ConnectEx(42, 43)would have caused aSystemError.)socket.getnameinfo((), 42)time.strftime('aoeu', ())time.asctime(())time.mktime(())
|
|
||
| if (!PyArg_ParseTuple(state, "OK", &buffer, &flag)) | ||
| if (!PyArg_ParseTuple(state, | ||
| "OK;IncrementalNewlineDecoder.setstate(): illegal " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If state is not a tuple, PyArg_ParseTuple() raises SystemError which means an internal error in the interpreter or extension. Add an explicit check and raise TypeError if state is not a tuple. See lin2adpcm() for example. This comment is applicable to other changes below.
And this error message looks too long to me. The method name (or at least the class name) could be omitted. The context can be derived from a traceback.
|
|
||
| if (!PyArg_ParseTuple(ftuple, "O&O", _get_name, &name, &dll)) { | ||
| if (!PyArg_ParseTuple(ftuple, "O&O;illegal func_spec argument", | ||
| _get_name, &name, &dll)) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of multiline condition the opening { should be moved to the separate line for better readability. This is a new requirement of PEP 7.
This comment is applicable to other changes below.
A Python core developer, serhiy-storchaka, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify serhiy-storchaka along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.
| channel = PyTuple_GetItem(samps, chan); | ||
| if (!PyTuple_Check(channel)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "ratecv(): channel must be a tuple"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure about this error message, as I failed to find the documentation of the state argument.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just "illegal state argument"? state is an opaque value. It is returned by ratecv() and passed to other invocation of ratecv(). "channel" is nowhere documented.
note that I also added very basic tests to test_audioop.py and to test_io.py, to verify SystemError is not raised anymore.
I didn't expect the Spanish Inquisition!
| channel = PyTuple_GetItem(samps, chan); | ||
| if (!PyTuple_Check(channel)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "ratecv(): channel must be a tuple"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just "illegal state argument"? state is an opaque value. It is returned by ratecv() and passed to other invocation of ratecv(). "channel" is nowhere documented.
| DWORD err; | ||
|
|
||
| if (!PyArg_ParseTuple(args, F_HANDLE "O", &ConnectSocket, &AddressObj)) | ||
| if (!PyArg_ParseTuple(args, F_HANDLE "O!:ConnectEx", &ConnectSocket, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not it look better if break a line before &ConnectSocket?
| return NULL; | ||
| } | ||
| else if (!gettmarg(tup, &buf) || !checktm(&buf)) | ||
| else if (!gettmarg(tup, &buf, "iiiiiiiii;strftime(): illegal time tuple " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it look better if break a line before the format argument and before !checktm(&buf)?
I think a NEWS.d entry for this PR is not required. But you can add it if you prefer.
|
|
||
| } else if (!gettmarg(tup, &buf, "iiiiiiiii;asctime(): illegal time tuple " | ||
| "argument") || !checktm(&buf)) | ||
| } else if (!gettmarg(tup, &buf, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet one nitpick: break a line between "}" and "else" while we are here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review, Serhiy!
This was referenced
This was referenced
