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

Remove uses of nb_long slot, and rename to nb_reserved. #49160

Closed
mdickinson opened this issue Jan 10, 2009 · 13 comments
Closed

Remove uses of nb_long slot, and rename to nb_reserved. #49160

mdickinson opened this issue Jan 10, 2009 · 13 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@mdickinson
Copy link
Member

BPO 4910
Nosy @mdickinson, @benjaminp
Files
  • issue4910_1.patch: Stage 1 of nb_long removal
  • issue4910_1a.patch: Stage 1 nb_long removal (improved patch)
  • issue4910_2.patch
  • issue4910_3.patch
  • issue4910_4.patch: deprecate PyNumber_Int
  • 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 = 'https://github.com/benjaminp'
    closed_at = <Date 2009-02-11.17:07:06.097>
    created_at = <Date 2009-01-10.20:32:22.374>
    labels = ['type-bug']
    title = 'Remove uses of nb_long slot, and rename to nb_reserved.'
    updated_at = <Date 2009-02-11.17:07:06.096>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2009-02-11.17:07:06.096>
    actor = 'mark.dickinson'
    assignee = 'benjamin.peterson'
    closed = True
    closed_date = <Date 2009-02-11.17:07:06.097>
    closer = 'mark.dickinson'
    components = []
    creation = <Date 2009-01-10.20:32:22.374>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['12696', '12697', '12754', '12756', '12781']
    hgrepos = []
    issue_num = 4910
    keywords = ['patch']
    message_count = 13.0
    messages = ['79571', '79581', '79655', '79659', '79689', '79902', '79907', '79913', '79918', '79973', '80031', '80055', '81652']
    nosy_count = 2.0
    nosy_names = ['mark.dickinson', 'benjamin.peterson']
    pr_nums = []
    priority = 'critical'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue4910'
    versions = ['Python 3.0', 'Python 3.1']

    @mdickinson
    Copy link
    Member Author

    In Python 3.x:

    >>> int.__long__.__doc__
    'x.__long__() <==> long(x)'

    But the long builtin no longer exists.

    I'm actually not sure why the nb_long slot still exists in 3.x at all.
    Can anyone enlighten me?

    @mdickinson
    Copy link
    Member Author

    long(x) replaced by int(x) in r68508.

    I'm still wondering about the nb_long slot, though.

    @mdickinson mdickinson changed the title help(int.__long__) refers to nonexistent long function Should both nb_long and nb_int still exist in 3.x? Jan 10, 2009
    @mdickinson
    Copy link
    Member Author

    Here's a patch against the py3k branch that gets rid of the two existing
    uses of nb_long in the core:

    • in PyNumber_Long, conversion was attempted first using nb_int and
      then using nb_long. The patch simply removes the nb_long code, so
      int(x) no longer attempts to use the __long__ method for conversion.

    • in Modules/_struct.c, there's a call to nb_long in a function that's
      attempting to turn an arbitrary PyObject into a PyLongObject; the
      patch replaces this with nb_int (and updates an error message).

    • In Lib/test/test_long.py, __long__ has been replaced with __int__
      in a test that __int__/long takes precedence over __trunc__
      for conversion to int.

    With this patch, all tests pass on my (OS X 10.5/Intel) machine.
    (Except test_socket, but I'm 97.2% certain that's unrelated.)

    If someone can review this quickly I'll move on to the next patch towards
    removing nb_long. (My issue bpo-1717 experience suggests that it ought to be
    easier to effect the removal via a series of 3 or 4 short, easy-to-review
    patches with clear intent than via one bigger, more confused patch.)

    I think it would be good if nb_long could be altered before 3.0.1.

    Benjamin, do you have time to take a look?

    @mdickinson mdickinson changed the title Should both nb_long and nb_int still exist in 3.x? Remove uses of nb_long slot, and rename to nb_reserved. Jan 12, 2009
    @mdickinson mdickinson added the type-bug An unexpected behavior, bug, or error label Jan 12, 2009
    @mdickinson
    Copy link
    Member Author

    That was a pretty poor patch. Here's a better one:

    • added Misc/NEWS entry
    • added tests to check that __long__ is never called
    • removed Modules/_struct.c change, in the interests of
      keeping the patch focused.

    @benjaminp
    Copy link
    Contributor

    The first installment looks good!

    @mdickinson
    Copy link
    Member Author

    Thanks, Benjamin! Checked in in r68553, backported to 3.0 in r68556.

    Here's the second patch, which fixes almost all remaining uses of nb_long
    but stops short of actually removing/renaming the nb_long slot.

    Notes:

    (1) I haven't tested the change to PC/winreg.c

    (2) The Modules/_struct.c change does introduce a change in behaviour:
    for example, before the patch,

    struct.pack('q', decimal.Decimal(1))

    raises struct.error. After the patch, the packing succeeds. I *think*
    the patched behaviour is probably the right behaviour, since it agrees
    with 2.x, but it's not 100% clear to me what the intentions of the struct
    module are with respect to integer packing of non-integer types. This is
    probably a question for another issue.

    @benjaminp
    Copy link
    Contributor

    On Thu, Jan 15, 2009 at 10:48 AM, Mark Dickinson <report@bugs.python.org> wrote:

    Mark Dickinson <dickinsm@gmail.com> added the comment:

    Thanks, Benjamin! Checked in in r68553, backported to 3.0 in r68556.

    Here's the second patch, which fixes almost all remaining uses of nb_long
    but stops short of actually removing/renaming the nb_long slot.

    Notes:

    (1) I haven't tested the change to PC/winreg.c

    This looks correct. In fact, I don't really see the point of having
    PyHKEY_unaryFailureFunc since a TypeError will automatically be raised
    if the slot is NULL, but that is certainly another issue.

    (2) The Modules/_struct.c change does introduce a change in behaviour:
    for example, before the patch,

    struct.pack('q', decimal.Decimal(1))

    raises struct.error. After the patch, the packing succeeds. I *think*
    the patched behaviour is probably the right behaviour, since it agrees
    with 2.x, but it's not 100% clear to me what the intentions of the struct
    module are with respect to integer packing of non-integer types. This is
    probably a question for another issue.

    Since Decimal implements __int__ and that's what the struct module is
    converting with, I think this is fine.

    Overall, the patch looks fine. I wonder if we should mark
    PyNumber_Long as deprecated in the docs, though.

    @mdickinson
    Copy link
    Member Author

    I wonder if we should mark
    PyNumber_Long as deprecated in the docs, though.

    It would be nice to standardize on one of (PyNumber_Long, PyNumber_Int)
    and document the other as deprecated.

    Maybe it would make more sense to stick with PyNumber_Long and deprecate
    PyNumber_Int, though? It would make 2.x -> 3.x transitions easier, and
    would be consistent with some of the rest of the API: we seem to be
    using PyLong_Check in preference to PyInt_Check in current code...

    @mdickinson
    Copy link
    Member Author

    Second patch applied in r68619 (py3k) and r68620 (3.0 maintenance branch).

    Here's the third and final patch: rename the nb_long slot to nb_reserved,
    and change its type to (void *).

    @benjaminp
    Copy link
    Contributor

    +1 for applying the last patch.

    @mdickinson
    Copy link
    Member Author

    Thanks again for reviewing, Benjamin. nb_long renamed in r68651, merged
    to 3.0 (along with a few other long->int cleanups) in r68666.

    There seems to be a consensus on deprecating PyNumber_Int. I'll leave
    this open until that's taken care of.

    @mdickinson
    Copy link
    Member Author

    Here's a patch that deprecates PyNumber_Int in favour of PyNumber_Long:

    • move PyNumber_Int definition from abstract.h to intobject.h (and
      fix up comments at the top of intobject.h)
    • mark PyNumber_Int as deprecated in the c-api docs, with a promise
      to remove it for 3.1.

    I suppose that in theory this goes too far: we should really deprecate in
    3.1 and remove in 3.2. But given all the other stuff that's going on for
    3.0.1, this doesn't seem too bad. Benjamin, what do you think?

    N.B. We should remember to actually remove intobject.h for 3.1. :)

    @mdickinson
    Copy link
    Member Author

    PyNumber_Int deprecated in r69517, r69518.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants