long long support for array module #41773
Comments
|
This patch adds signed and unsigned long long support Also corrects a minor bug in PyLong_AsUnsignedLongLong |
|
Logged In: YES No two conversion function in longobject.c seem to have the same rules for what to do about non-long objects :-( I'm afraid some clean-up would be useful, but also difficult for fear of breaking existing user C code :-( In fact, your patch doesn't apply with today's CVS because someone already tried to add some magic in PyObject_AsLongLong(). It also fails on test_array.py and applies uncleanly on arraymodule.c. Also, it needs to update the array module documentation. |
|
Logged In: YES My patch was against 2.4... (duh!) The other bug is already fixed on 2.5. |
devdanzin
mannequin
added
extension-modules
|
How about this patch? I haven't tested so intensely, but testcase seems |
|
So it looks as though this isn't going in to Python 2.7. How about 3.x? |
|
Seems like a reasonable addition to me. Anyone feel like refreshing the patch so that it applies to py3k? |
|
BTW, the PyLong_AsUnsignedLongLong BadInternalCall has long since disappeared. I agree with Armin Rigo that the conversion functions in longobject.c are a mess, though (and also that cleanup is difficult). |
|
Overall the patch looks good. I don't think it is an extremely important feature, but similar support is already available in other places (e.g. 'struct', 'ctypes'). Here is a patch updated for py3k with some minor additions: (1) Fixed some doc inconsistencies. (2) needs unit tests if possible. If anyone has any good ideas on how to test, then I would be happy to implement the tests. |
|
This is an important feature to me. Now I get to redo a bunch of code to have two completely different code paths to do the same thing because nobody could be bothered to keep array up-to-date. |
|
Here is a refresh of this patch for 3.3. Please review. |
|
+if have_long_long: It is maybe better to use @unittest.skipIf(not have_long_long, 'need long long support'). Except of this nit, the patch looks correct. |
|
Victor, I like the decorator approach much better. Thanks. Attached is a new patch with that update. |
|
I left some remarks on Rietveld. |
|
I made the observation on Rietveld that the following code is never http://bugs.python.org/review/1172711/diff/3310/10310#newcode394 Meador correctly pointed out that the code allows for duck typing. >>> import array, struct
>>> a = array.array('L', [1,2,3])
>>> class T(object):
... def __init__(self, value):
... self.value = value
... def __int__(self):
... return self.value
...
>>> a = array.array('L', [1,2,3])
>>> struct.pack_into('L', a, 0, 9)
>>> a
array('L', [9, 2, 3])
>>> a[0] = T(100)
>>> a
array('L', [100, 2, 3])
>>> struct.pack_into('L', a, 0, T(200))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
struct.error: required argument is not an integer
>>>I vastly prefer the struct module behavior. Since the code isn't executed Is it really the intention for array to allow duck typing? The documentation "This module defines an object type which can compactly represent an array "Basic value" doesn't sound to me like "anything that has an __int__() method". Also, consider this: >>> sum([T(1),T(2),T(3)])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'int' and 'T'
>>> sum(array.array('L', [T(1),T(2),T(3)]))
6 |
>>>> import array, struct
>>>> a = array.array('L', [1,2,3])
>>>> class T(object):
> ... def __init__(self, value):
> ... self.value = value
> ... def __int__(self):
> ... return self.value
> ...
>>>> a = array.array('L', [1,2,3])
>>>> struct.pack_into('L', a, 0, 9)
>>>> a
> array('L', [9, 2, 3])
>>>> a[0] = T(100)
>>>> a
> array('L', [100, 2, 3])
>>>> struct.pack_into('L', a, 0, T(200))
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> struct.error: required argument is not an integer
>>>>
>
> I vastly prefer the struct module behavior. Since the code isn't executed
> by any tests:Yeah, but if it is a good feature we can always add more tests. I think the ... def __init__(self, value):
... self.value = value
... def __int__(self):
... return self.value
... def __index__(self):
... return self.value
...
>>> a = array.array('L', [1,2,3])
>>> struct.pack_into('L', a, 0, T(200))
>>> a
array('L', [200, 2, 3])Also, check out bpo-1530559. Originally, struct did allow the IMO, having some way to convert objects to integers is a nice feature |
|
Yes, please let's not add any new __int__-based duck typing here; IMO, we should be moving away from such uses of __int__. I'd be fine with __index__ based duck-typing. |
Mark, just to clarify a bit, the behavior is already there in the array module (by way of 'PyLong_AsLong'). The fact that it is there was picked up on a code review for this issue. Anyway, I think we should open a new issue to track the '__index__' vs. '__int__' stuff. |
Okay, understood. But the new 'long long' support provided by this patch still allows for __int__-based duck typing, right? >>> array('Q', [1, 2, Decimal(3.2)])
array('Q', [1, 2, 3])That's the new duck typing I meant. I see this acceptance of things with an __int__ method as a mistake, and my gut reaction earlier was that it seems wrong to propagate that mistake into the new long long functionality, even though it's already present in other places in the array module. On second thoughts though, it would be a peculiar inconsistency to be able to pass Decimal objects to array('L', ...) but not to array('Q', ...). So probably better to accept this behaviour for now, and open another issue for the __int__ / __index__ discussion, as you suggest. BTW, the patch and tests look good to me, and all tests pass here (OS X !0.6, 64-bit) (Well, not quite true, but I fail to see how these changes could be responsible for the test_socket and test_packaging failures I'm seeing :-). I get compile-time warnings from the 'int' declarations that should be 'Py_ssize_t', but I understand that's taken care of already... |
Yes, but ...
... I had this inconsistency in mind. I opened bpo-12974 for the Now we just have to figure out which issue gets fixed first :-D I am |
|
Updated patch with the 'Py_ssize_t' fixes. |
|
@meadori: please write the version of your patch directly in the filename. For example, I use the pattern: name.patch, name-2.patch, name-3.patch, ... |
I also think this should be committed first. |
|
Agreed. Commit first; worry about __int__ and __index__ later. |
|
New changeset 15659e0e2b2e by Meador Inge in branch 'default': |
|
New changeset 3c56e546dc60 by Victor Stinner in branch 'default': |
|
New changeset 672b63aff0f4 by Meador Inge in branch 'default': |
Woops, I wrote the wrong module name. Thanks for fixing it. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: