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

negative PyLong -> C unsigned integral, TypeError or OverflowError? #49425

Closed
dalcinl mannequin opened this issue Feb 6, 2009 · 10 comments
Closed

negative PyLong -> C unsigned integral, TypeError or OverflowError? #49425

dalcinl mannequin opened this issue Feb 6, 2009 · 10 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@dalcinl
Copy link
Mannequin

dalcinl mannequin commented Feb 6, 2009

BPO 5175
Nosy @tim-one, @mdickinson
Files
  • negative-to-unsigned.diff
  • negative-to-unsigned2.diff
  • setup.py.diff
  • 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/mdickinson'
    closed_at = <Date 2009-02-10.16:26:37.564>
    created_at = <Date 2009-02-06.23:36:42.867>
    labels = ['interpreter-core', 'type-bug']
    title = 'negative PyLong -> C unsigned integral, TypeError or OverflowError?'
    updated_at = <Date 2009-02-10.16:26:37.563>
    user = 'https://bugs.python.org/dalcinl'

    bugs.python.org fields:

    activity = <Date 2009-02-10.16:26:37.563>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2009-02-10.16:26:37.564>
    closer = 'mark.dickinson'
    components = ['Interpreter Core']
    creation = <Date 2009-02-06.23:36:42.867>
    creator = 'dalcinl'
    dependencies = []
    files = ['13010', '13012', '13014']
    hgrepos = []
    issue_num = 5175
    keywords = ['patch']
    message_count = 10.0
    messages = ['81316', '81376', '81377', '81391', '81477', '81540', '81550', '81554', '81556', '81557']
    nosy_count = 3.0
    nosy_names = ['tim.peters', 'mark.dickinson', 'dalcinl']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5175'
    versions = ['Python 3.1', 'Python 2.7']

    @dalcinl
    Copy link
    Mannequin Author

    dalcinl mannequin commented Feb 6, 2009

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

    @dalcinl dalcinl mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Feb 6, 2009
    @mdickinson mdickinson self-assigned this Feb 7, 2009
    @mdickinson
    Copy link
    Member

    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.

    @mdickinson
    Copy link
    Member

    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.

    @dalcinl
    Copy link
    Mannequin Author

    dalcinl mannequin commented Feb 8, 2009

    I can contribute a patch. However, I would like to wait until Tim
    comments on this.

    @mdickinson
    Copy link
    Member

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

    @dalcinl
    Copy link
    Mannequin Author

    dalcinl mannequin commented Feb 10, 2009

    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)

    @mdickinson
    Copy link
    Member

    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?

    @dalcinl
    Copy link
    Mannequin Author

    dalcinl mannequin commented Feb 10, 2009

    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.

    @mdickinson
    Copy link
    Member

    Committed, r69498 (trunk) and r69499 (py3k).

    @mdickinson
    Copy link
    Member

    ...and your patch for setup.py applied in r69500, r69501, r69502, r69503.

    Thank you!

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant