classification
Title: negative PyLong -> C unsigned integral, TypeError or OverflowError?
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.1, Python 2.7
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: dalcinl, mark.dickinson, tim.peters
Priority: normal Keywords: patch

Created on 2009-02-06 23:36 by dalcinl, last changed 2009-02-10 16:26 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
negative-to-unsigned.diff dalcinl, 2009-02-10 11:47
negative-to-unsigned2.diff mark.dickinson, 2009-02-10 13:50
setup.py.diff dalcinl, 2009-02-10 15:01
Messages (10)
msg81316 - (view) Author: Lisandro Dalcin (dalcinl) Date: 2009-02-06 23:36
At Objects/longobject.c, in almost all cases
OverflowError is raised when a unsigned integral is requested from a
negative PyLong. However, this one breaks the rules:

int
_PyLong_AsByteArray(PyLongObject* v,
                  unsigned char* bytes, size_t n,
                  int little_endian, int is_signed)
{
<...>
              if (!is_signed) {
                      PyErr_SetString(PyExc_TypeError,
                              "can't convert negative long to 
unsigned");
                      return -1;
              }
<...>
}
msg81376 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-08 12:45
This also affects 3.1.

Note that the current behaviour (or rather, its effects in 
PyLong_AsUnsignedLongLong) is as documented.  In

http://docs.python.org/dev/c-api/long.html

it says for PyLong_AsUnsignedLongLong:

"Return a C unsigned long long from a Python long integer. If pylong 
cannot be represented as an unsigned long long, an OverflowError will be 
raised if the value is positive, or a TypeError will be raised if the 
value is negative."

...which suggests that the choice of TypeError was intentional.  It 
still seems wrong to me, though: the argument has the correct type, but 
an illegal value, so ValueError or (for consistency with the other 
methods) OverflowError would seem more appropriate.

If this change is made, then test_struct needs fixing: the change 
affects the 'Q' struct format.  I don't think the change in struct 
behaviour is serious, since there's very little consistency between 
different struct types at the moment:  for negative ints, 'Q' raises an 
error, 'L' and 'I' give a DeprecationWarning, and 'H' raises a 
struct.error.
msg81377 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-08 13:00
Looks like this was changed in a checkin by Tim Peters in r21099;  before 
r21099, PyLong_AsUnsignedLongLong raised OverflowError for negative 
numbers.  After the checkin, it raised TypeError.  I suspect the change 
was inadvertent.

Tim, any comments?

Lisandro, do you have any interest in contributing a patch for this?  The 
patch should change the exception type, fix the docs, add a test or two 
for the fixed behaviour of PyLong_AsUnsignedLongLong to 
Modules/_testcapimodule.c, and fix the test_struct test for the 'Q' format 
code.
msg81391 - (view) Author: Lisandro Dalcin (dalcinl) Date: 2009-02-08 17:04
I can contribute a patch. However, I would like to wait until Tim 
comments on this.
msg81477 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-09 19:22
> However, I would like to wait until Tim comments on this.

You may be in for a long wait!  I hesitate to make the heretical 
suggestion that there may be more important things in life than fixing 
minor inconsistencies in Python, but I think it's possible that Tim has 
found some.  :-)
msg81540 - (view) Author: Lisandro Dalcin (dalcinl) Date: 2009-02-10 11:47
Mark, here you have a patch. I've only 'make test' on a 32bit Linux box

Just two comments:

- in docs: perhaps the 'versionchanged' stuff should be added.

- in tests: I did not touch Modules/_testcapimodule.c, as it seems the
test is covered. However, note that in all these tests, actual exception
types are not checked)
msg81550 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-10 13:50
Thanks for the patch!

I agree that the 'versionchanged' reference should be added in the docs.
 I also think that the test should be updated to check the exception type.

Here's a modified version of your patch that adds a test for
OverflowError to the capi tests, adds 'versionchanged', and rewords the
docs slightly.  I also took the liberty of rewording the docs for
PyLong_AsLongLong, to match.  I've tested this on a 64-bit linux
machine, and checked that the docs build properly.  I'll test on 32-bit
and 64-bit OS X later today.

Lisandro, does this updated patch work for you?
msg81554 - (view) Author: Lisandro Dalcin (dalcinl) Date: 2009-02-10 15:01
It worked for me.

BTW, 'make test' did not noticed the change in Modules/testcapi_long.h,
which is #include'd by Modules/_testcapimodule.c. I've attached a
trivial patch for setup.py fixing the dependency issue.
msg81556 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-10 16:14
Committed, r69498 (trunk) and r69499 (py3k).
msg81557 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-10 16:26
...and your patch for setup.py applied in r69500, r69501, r69502, r69503.

Thank you!
History
Date User Action Args
2009-02-10 16:26:37mark.dickinsonsetstatus: open -> closed
resolution: accepted
messages: + msg81557
2009-02-10 16:14:26mark.dickinsonsetmessages: + msg81556
2009-02-10 15:01:48dalcinlsetfiles: + setup.py.diff
messages: + msg81554
2009-02-10 13:50:09mark.dickinsonsetfiles: + negative-to-unsigned2.diff
messages: + msg81550
2009-02-10 11:47:03dalcinlsetfiles: + negative-to-unsigned.diff
keywords: + patch
messages: + msg81540
2009-02-09 19:22:41mark.dickinsonsetmessages: + msg81477
2009-02-08 17:04:58dalcinlsetmessages: + msg81391
2009-02-08 13:00:11mark.dickinsonsetnosy: + tim.peters
messages: + msg81377
2009-02-08 12:45:40mark.dickinsonsetpriority: normal
messages: + msg81376
versions: + Python 3.1
2009-02-07 08:03:01mark.dickinsonsetassignee: mark.dickinson
nosy: + mark.dickinson
2009-02-06 23:36:42dalcinlcreate