Skip to content
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

long long support for array module #41773

Closed
orenti mannequin opened this issue Mar 29, 2005 · 27 comments
Closed

long long support for array module #41773

orenti mannequin opened this issue Mar 29, 2005 · 27 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@orenti
Copy link
Mannequin

orenti mannequin commented Mar 29, 2005

BPO 1172711
Nosy @mdickinson, @vstinner, @skrah, @meadori
Files
  • arraymodule-longlong.diff
  • arraymodule-longlong-2.5.diff
  • array_long_long.patch: for trunk
  • issue-1172711.patch: py3k patch
  • issue-1172711.patch: Patch against tip (3.3.0a0)
  • issue-1172711.patch: v2 patch against tip (3.3.0a0)
  • issue-1172711.patch: v3 patch against tip (3.3.0a0)
  • 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:

    assignee = None
    closed_at = <Date 2011-09-21.01:06:14.909>
    created_at = <Date 2005-03-29.18:58:42.000>
    labels = ['extension-modules', 'type-feature']
    title = 'long long support for array module'
    updated_at = <Date 2011-09-21.08:14:34.741>
    user = 'https://bugs.python.org/orenti'

    bugs.python.org fields:

    activity = <Date 2011-09-21.08:14:34.741>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-09-21.01:06:14.909>
    closer = 'meador.inge'
    components = ['Extension Modules']
    creation = <Date 2005-03-29.18:58:42.000>
    creator = 'orenti'
    dependencies = []
    files = ['6576', '6577', '14439', '18627', '23100', '23121', '23148']
    hgrepos = []
    issue_num = 1172711
    keywords = ['patch']
    message_count = 27.0
    messages = ['48085', '48086', '48087', '90059', '108500', '108508', '108509', '114784', '126142', '143507', '143745', '143751', '143856', '143934', '143948', '143955', '143980', '143983', '144001', '144003', '144062', '144121', '144125', '144359', '144360', '144364', '144371']
    nosy_count = 9.0
    nosy_names = ['orenti', 'mark.dickinson', 'vstinner', 'ocean-city', 'cmcqueen1975', 'skrah', 'meador.inge', 'mattchaput', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1172711'
    versions = ['Python 3.3']

    @orenti
    Copy link
    Mannequin Author

    orenti mannequin commented Mar 29, 2005

    This patch adds signed and unsigned long long support
    to the array module. These types are already supported
    by the struct module and use the same format characters
    (q/Q).

    Also corrects a minor bug in PyLong_AsUnsignedLongLong
    which reports a BadInternalCall for arguments of
    inappropriate type rather than a mere TypeError as
    reported by the other conversion functions.

    @orenti orenti mannequin added the stdlib Python modules in the Lib dir label Mar 29, 2005
    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Apr 3, 2005

    Logged In: YES
    user_id=4771

    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.

    @orenti
    Copy link
    Mannequin Author

    orenti mannequin commented Apr 7, 2005

    Logged In: YES
    user_id=562624

    My patch was against 2.4... (duh!)

    The other bug is already fixed on 2.5.

    @devdanzin devdanzin mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed stdlib Python modules in the Lib dir labels Feb 15, 2009
    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Jul 3, 2009

    How about this patch? I haven't tested so intensely, but testcase seems
    working.

    @cmcqueen1975
    Copy link
    Mannequin

    cmcqueen1975 mannequin commented Jun 24, 2010

    So it looks as though this isn't going in to Python 2.7.

    How about 3.x?

    @mdickinson
    Copy link
    Member

    Seems like a reasonable addition to me. Anyone feel like refreshing the patch so that it applies to py3k?

    @mdickinson
    Copy link
    Member

    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).

    @meadori
    Copy link
    Member

    meadori commented Aug 24, 2010

    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) Added pickling support for the new type codes. The special
    pickling support looks only to be in py3k.

    (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.

    @mattchaput
    Copy link
    Mannequin

    mattchaput mannequin commented Jan 12, 2011

    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.

    @meadori
    Copy link
    Member

    meadori commented Sep 5, 2011

    Here is a refresh of this patch for 3.3. Please review.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 8, 2011

    +if have_long_long:
    + class LongLongTest(SignedNumberTest):
    + ...

    It is maybe better to use @unittest.skipIf(not have_long_long, 'need long long support'). Except of this nit, the patch looks correct.

    @meadori
    Copy link
    Member

    meadori commented Sep 9, 2011

    Victor, I like the decorator approach much better. Thanks. Attached is a new patch with that update.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 11, 2011

    I left some remarks on Rietveld.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 12, 2011

    I made the observation on Rietveld that the following code is never
    executed by the test suite. The same applies to similar existing
    passages in arraymodule.c:

    http://bugs.python.org/review/1172711/diff/3310/10310#newcode394

    Meador correctly pointed out that the code allows for duck typing.
    But the struct module (and by extension memoryview that must follow
    the struct module) don't:

    >>> 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:

    Is it really the intention for array to allow duck typing? The documentation
    says:

    "This module defines an object type which can compactly represent an array
    of basic values: characters, integers, floating point numbers."

    "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

    @meadori
    Copy link
    Member

    meadori commented Sep 13, 2011

    >>>> 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
    real issue is whether or not this behavior is even desirable. Also, similar
    behavior can be achieved with struct by using '__index__':

    ...      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
    '__int__' and '__long__' behavior, but it was deprecated and replaced
    with '__index__'. Maybe we should do the same for array?

    IMO, having some way to convert objects to integers is a nice feature
    and I think we will find more cases like the PyCUDA case from
    bpo-1530559 where folks need this.

    @mdickinson
    Copy link
    Member

    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.

    @meadori
    Copy link
    Member

    meadori commented Sep 13, 2011

    Yes, please let's not add any new __int__-based duck typing here;

    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.

    @mdickinson
    Copy link
    Member

    Mark, just to clarify a bit, the behavior is already there in the array module

    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...

    @meadori
    Copy link
    Member

    meadori commented Sep 14, 2011

    Okay, understood.  But the new 'long long' support provided by this patch still allows for __int__-based duck typing, right?

    Yes, but ...

    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.

    ... I had this inconsistency in mind. I opened bpo-12974 for the
    __int__/index problem.

    Now we just have to figure out which issue gets fixed first :-D I am
    OK with applying the fix for this issue first.

    @meadori
    Copy link
    Member

    meadori commented Sep 14, 2011

    Updated patch with the 'Py_ssize_t' fixes.

    @vstinner
    Copy link
    Member

    @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, ...

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 16, 2011

    I am OK with applying the fix for this issue first.

    I also think this should be committed first.

    @mdickinson
    Copy link
    Member

    Agreed. Commit first; worry about __int__ and __index__ later.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 21, 2011

    New changeset 15659e0e2b2e by Meador Inge in branch 'default':
    Issue bpo-1172711: Add 'long long' support to the array module.
    http://hg.python.org/cpython/rev/15659e0e2b2e

    @meadori meadori closed this as completed Sep 21, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 21, 2011

    New changeset 3c56e546dc60 by Victor Stinner in branch 'default':
    Issue bpo-1172711: Update What's New in Python 3.3 document for the struct module
    http://hg.python.org/cpython/rev/3c56e546dc60

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 21, 2011

    New changeset 672b63aff0f4 by Meador Inge in branch 'default':
    Issue bpo-1172711: Update What's New in Python 3.3 document for the array module.
    http://hg.python.org/cpython/rev/672b63aff0f4

    @vstinner
    Copy link
    Member

    New changeset 672b63aff0f4 by Meador Inge in branch 'default'

    Woops, I wrote the wrong module name. Thanks for fixing it.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants