-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
bpo-29843: raise AttributeError if given negative _length_ #10029
bpo-29843: raise AttributeError if given negative _length_ #10029
Conversation
Also |
The issue number looks wrong. |
Fixed. |
ba7d522
to
e8d54b5
Compare
@serhiy-storchaka, I've addressed the exception handling and raised exception types as per your comment. Please take another look! |
Lib/ctypes/test/test_arrays.py
Outdated
with self.assertRaises(OverflowError): | ||
|
||
def test_bad_length(self): | ||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is imported at the top of the file.
@@ -0,0 +1,4 @@ | |||
Raise `ValueError` instead of `OverflowError` in case of a negative | |||
``_length_`` in a `ctypes.Array` subclass. Also raise `TypeError` | |||
instead of `OverflowError` for non-integer ``_length_``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"instead of AttributeError".
@@ -0,0 +1,4 @@ | |||
Raise `ValueError` instead of `OverflowError` in case of a negative |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues with using the default role. It is better to use :exc:`ValueError`
or ``ValueError``
or just remove mark up.
Modules/_ctypes/_ctypes.c
Outdated
"which must be a positive integer"); | ||
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (!PyErr_Occurred() || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyErr_Occurred()
should always return true here. Remove this redundant check.
Modules/_ctypes/_ctypes.c
Outdated
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (!PyErr_Occurred() || | ||
PyErr_ExceptionMatches(PyExc_AttributeError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worth to add similar check for _type_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a new PR for that. Should I also make a new issue?
@serhiy-storchaka, I've made the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
Modules/_ctypes/_ctypes.c
Outdated
"which must be a positive integer"); | ||
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put {
on the same line as if
.
Modules/_ctypes/_ctypes.c
Outdated
"which must be a positive integer"); | ||
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put {
on the same line as if
. It should be on a new line only for multiline conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a tiny nit.
Modules/_ctypes/_ctypes.c
Outdated
"which must be a positive integer"); | ||
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put {
on the same line as if
. It should be on a new line only for multiline conditions.
Modules/_ctypes/_ctypes.c
Outdated
"which must be a positive integer"); | ||
Py_XDECREF(length_attr); | ||
if (!length_attr) { | ||
if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put {
on the same line as if
. It should be on a new line only for multiline conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a nitpick.
Should this be backported, and if so, how far back? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Hum, GitHub is sick today. I approved the PR again :-) |
All right until GitHub allow to merge the PR twice. 😉 |
This is based on @orenmn's PR #3822.
The tests are the same, but this uses
_PyLong_Sign()
to check for negative values rather thanPyLong_AsLongAndOverflow()
.This raises
AttributeError
since the existing code already raises that for other invalid values (non-integers).https://bugs.python.org/issue29843