-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-141578: Improve overflow error messages in PyLong_AsLong and PyLong_AsInt #141585
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
base: main
Are you sure you want to change the base?
gh-141578: Improve overflow error messages in PyLong_AsLong and PyLong_AsInt #141585
Conversation
2469099 to
c700fa6
Compare
c700fa6 to
dc88dc0
Compare
| } | ||
| else { | ||
| PyErr_Format(PyExc_OverflowError, | ||
| "Python int too large to convert to C long: " |
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.
Maybe 'too small' reads better here (and below)?
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.
Could you be more precise? Replacing "too large" with "too small" will be just plain wrong right here.
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.
We are in the negative case here and "too large" combined with "less than" rings a bell for me. Maybe just splitting hairs (or I am completely mistaken).
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.
My english is bad, so I trust your judgement.
Though, I don't see a problem so far. This exception supposed to
mean: value too large (by magnitude) for long, it would be < LONG_MIN.
| "Python int too large to convert to C long: " | |
| "Python int magnitude too large to convert to C long: " |
?
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.
See Serhiy's comment below. I agree it would be better to just have simpler and unified messages.
serhiy-storchaka
left a comment
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.
In Python/getargs.c, other messages are used, like "signed short integer is less than minimum". I think it is incorrect (signed short integer cannot be less than SHRT_MIN). "integer is less than minimum for C short int" looks more correct.
And some error messages are simply "integer value out of range" because when we use PyLong_AsNativeBytes() we cannot know whether it is too large or too small.
It would be better to produce unified error messages in PyLong_As*(), PyArg_Parse*(), etc.
Maybe it's better. I.e.:
I think this is out of the scope for this pr. |
Not if we just use Line 1259 in dc88dc0
like in _PyLong_AsByteArray which is called in PyLong_AsLongLong.
Then at least those would be in sync? |
|
Yes, just removing XXX marks is an alternative. |
Replace generic overflow messages with specific ones that indicate whether the value exceeds maximum or minimum bounds.
Fixes #141578