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

Argument Clinic: add unsigned integers converters #64459

Closed
serhiy-storchaka opened this issue Jan 14, 2014 · 15 comments
Closed

Argument Clinic: add unsigned integers converters #64459

serhiy-storchaka opened this issue Jan 14, 2014 · 15 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-argument-clinic type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 20260
Nosy @birkenfeld, @mdickinson, @larryhastings, @serhiy-storchaka, @MojoVampire
PRs
  • bpo-20260: Implement non-bitwise unsigned int converters for Argument Clinic. #8434
  • Files
  • clinic_unsigned_converters.patch
  • clinic_unsigned_converters.patch
  • 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 2018-07-26.11:35:25.681>
    created_at = <Date 2014-01-14.17:31:58.746>
    labels = ['interpreter-core', 'type-feature', '3.8', 'expert-argument-clinic']
    title = 'Argument Clinic: add unsigned integers converters'
    updated_at = <Date 2018-07-26.11:35:25.679>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2018-07-26.11:35:25.679>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-26.11:35:25.681>
    closer = 'serhiy.storchaka'
    components = ['Demos and Tools', 'Interpreter Core', 'Argument Clinic']
    creation = <Date 2014-01-14.17:31:58.746>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['33464', '33483']
    hgrepos = []
    issue_num = 20260
    keywords = ['patch']
    message_count = 15.0
    messages = ['208102', '208103', '208107', '208174', '208180', '208181', '208201', '208229', '208376', '208730', '208731', '208734', '208778', '209092', '322411']
    nosy_count = 6.0
    nosy_names = ['georg.brandl', 'mark.dickinson', 'larry', 'rmsr', 'serhiy.storchaka', 'josh.r']
    pr_nums = ['8434']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20260'
    versions = ['Python 3.8']

    @serhiy-storchaka
    Copy link
    Member Author

    Proposed patch adds support for non-bitwise unsigned integer arguments in Argument Clinic. I.e. now unsigned_int(bitwise=False) (or just unsigned_int) converts Python int in range from 0 to UINT_MAX to C unsigned int. Added support for unsigned_short, unsigned_int, unsigned_long, unsigned_PY_LONG_LONG, and size_t.

    Also added global private functions _Py_UnsignedShort_Converter(), _Py_UnsignedInt_Converter(), _Py_UnsignedLong_Converter(), _Py_UnsignedLongLong_Converter(), and _Py_Size_t_Converter(), which are used by Argument Clinic and in C code still not converted to Argument Clinic.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Jan 14, 2014
    @larryhastings
    Copy link
    Contributor

    Very nice! I had an idea to do this in the back of my head too. But my plate is already full.

    I haven't reviewed the patch yet--and I have a huge backlog of reviews already. But I can give this higher priority if you need it for other patches. When do you need this reviewed?

    @serhiy-storchaka
    Copy link
    Member Author

    Currently these converters are used only in zlib (unsigned int) and select (unsigned short). But perhaps during conversion to Argument Clinic we will discover that they are more appropriate than bitwise converters in other places. So there is no hurry.

    @larryhastings
    Copy link
    Contributor

    We can use this now. So I bumped it to the top of my queue and reviewed it. I had only minor feedback--thanks!

    @serhiy-storchaka
    Copy link
    Member Author

    Unfortunately I have discovered that there is significant difference between uint_converter in Modules/zlibmodule.c and proposed _PyLong_UnsignedInt_Converter. And there are tests in Lib/test/test_zlib.py which fail when _PyLong_UnsignedInt_Converter is used instead of uint_converter. uint_converter raises ValueError for negative integers, and _PyLong_UnsignedInt_Converter raises OverflowError.

    Possible solutions:

    1. Change tests. I don't like this, ValueError looks reasonable for these cases.

    2. Continue to use uint_converter in Modules/zlibmodule.c. Then new converters become less useful.

    3. Raise ValueError in new converters for negative integers. ValueError looks reasonable in many cases.

    4. Raise ValueError in PyLong_AsUnsignedLong etc for negative integers.

    Here is a patch which incorporates Larry's suggestions and implements option #3.

    @serhiy-storchaka
    Copy link
    Member Author

    Oh, and of course 5th option: do nothing.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 15, 2014

    I think we cannot change PyLong_AsUnsignedLong() without a deprecation period,
    if at all. That leaves the option of changing the converters.

    My preference is to raise either OverflowError or ValueError for *both*
    out-of-range conditions.

    People may me used to OverflowError by now -- the usage in PyLong_AsUnsignedLong()
    dates back to very early revisions -- but there are equally good reasons to use
    ValueError.

    @larryhastings
    Copy link
    Contributor

    I'm glad you caught that! First things first: the converted code should behave identically to the existing code, including raising the same exceptions.

    If you examine the exception hierarchy:
    http://docs.python.org/3.4/library/exceptions.html#exception-hierarchy

    you'll see that "OverflowError" is a subclass of "ArithmeticError". In other words, it represents when you perform an arithmetic operation that overflows the result type. Using it to also represent "you specified a value that is out of range for this conversion" seems wrong.

    So I like #3 as well.

    Could _PyLong_UnsignedInt_Converter catch the OverflowError raised by PyLong_AsUnsignedLong and reraise it as ValueError?

    @rmsr
    Copy link
    Mannequin

    rmsr mannequin commented Jan 18, 2014

    socketmodule has three builtins which use PyLong_AsUnsignedLong on their arguments and would benefit from these converters, but only if they raise OverflowError. So I vote for #2.

    I don't think it's unreasonable to continue to have locally-defined argument converters.

    @larryhastings
    Copy link
    Contributor

    Is this waiting on something? I agree that we can't change the behavior
    of existing functions. But your new converters should raise ValueError.

    @larryhastings
    Copy link
    Contributor

    Also, you didn't remove the _ in front of "Converter" in the names, e.g "_PyLong_UnsignedShort_Converter" should be "_PyLong_UnsignedShortConverter".

    @larryhastings
    Copy link
    Contributor

    Actually, here's another data point to consider. The underlying implementation of PyArg_Parse* is the function convertsimple() in Python/getargs.c. For all the non-bitwise integer format units ('bhi'), if they overflow or underflow the native integer they raise OverflowError.

    @serhiy-storchaka
    Copy link
    Member Author

    I think that we do not yet have enough data. Too little cases are known for which unsigned integer converters are needed, and different cases require a different behavior. I prefer to defer until we convert the majority of the code. Then we will see what behavior is most prevalent. We even can change and unify behavior in 3.5.

    There is no hurry. We can use in each module its own Converter (if needed). There should not be many such places.

    @larryhastings
    Copy link
    Contributor

    I can start citing data points if you like.

    socket.if_indextoname just calls PyLong_AsUnsignedLong(). Not sure what that throws.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 7cb7bcf by Serhiy Storchaka in branch 'master':
    bpo-20260: Implement non-bitwise unsigned int converters for Argument Clinic. (GH-8434)
    7cb7bcf

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Jul 26, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    AA-Turner pushed a commit to AA-Turner/devguide that referenced this issue Sep 13, 2023
    erlend-aasland pushed a commit to python/devguide that referenced this issue Sep 26, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-argument-clinic type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants