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

Jump to bottom

Add PyFloat_(Pack|Unpack)(2|4|8) to the public C API #91062

Closed
methane opened this issue Mar 3, 2022 · 12 comments
Closed

Add PyFloat_(Pack|Unpack)(2|4|8) to the public C API #91062

methane opened this issue Mar 3, 2022 · 12 comments
Labels
3.11 only security fixes topic-C-API

Comments

@methane
Copy link
Member

methane commented Mar 3, 2022

BPO 46906
Nosy @malemburg, @rhettinger, @mdickinson, @vstinner, @methane
PRs
  • bpo-46906: Move _PyFloat_{Pack,Unpack}{4,8} from internal to cpython #31649
  • bpo-46906: Add PyFloat_Pack8() to the C API #31657
  • bpo-46906: Mention PY_BIG_ENDIAN in PyFloat_Pack8() doc #31832
  • bpo-46906: Mention native endian in PyFloat_Pack8() doc #31866
    		•

    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 2022-03-14.15:58:36.669>
created_at = <Date 2022-03-03.03:09:32.087>
labels = ['expert-C-API', '3.11']
title = 'Add PyFloat_(Pack|Unpack)(2|4|8) to the public C API'
updated_at = <Date 2022-03-14.15:58:36.668>
user = 'https://github.com/methane'

    bugs.python.org fields:

    activity = <Date 2022-03-14.15:58:36.668>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2022-03-14.15:58:36.669>
closer = 'vstinner'
components = ['C API']
creation = <Date 2022-03-03.03:09:32.087>
creator = 'methane'
dependencies = []
files = []
hgrepos = []
issue_num = 46906
keywords = ['patch']
message_count = 12.0
messages = ['414401', '414410', '414411', '414416', '414419', '414427', '414944', '414952', '414953', '414954', '415151', '415152']
nosy_count = 6.0
nosy_names = ['lemburg', 'rhettinger', 'mark.dickinson', 'vstinner', 'stutzbach', 'methane']
pr_nums = ['31649', '31657', '31832', '31866']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46906'
versions = ['Python 3.11']
    @methane
    Copy link
    Member Author

    methane commented Mar 3, 2022

    Original issue. msgpack/msgpack-python#497

    _PyFloat_(Pack|Unpack)(4|8) is very nice API for serializers like msgpack.
    Converting double and float into char[] is not trivial and these APIs do it in very efficient way.

    And these APIs don't reveal CPython internal strucutre. It just convert double and float into char[].

    So please keep these APIs public for libraries like msgpack.

    @methane methane added 3.11 only security fixes topic-C-API labels Mar 3, 2022
    @vstinner
    Copy link
    Member

    vstinner commented Mar 3, 2022

    I disagree. A function should be either fully public: tested, documented. Or fully internal.

    In the past, many functions were in both, in the gray area.

    If these functions are useful, please make them *public* and document them. I'm +1 to make these functions public.

    @methane
    Copy link
    Member Author

    methane commented Mar 3, 2022

    OK. By quick grepping, I found only msgpack and bitstruct use these API.
    It is not enough number to make them public.

    @methane methane closed this as completed Mar 3, 2022
    @methane methane closed this as completed Mar 3, 2022
    @vstinner
    Copy link
    Member

    vstinner commented Mar 3, 2022

    I reopen the issue, I would like to make these functions public :-) I searched for the 6 functions moved to the internal C API and I found 6 projets using it in the top 5000 PyPI projects (at 2022-01-26):

    • msgpack
    • ddtrace
    • bitstruct
    • pickle5
    • line_profiler
    • zodbpickle

    The 6 functions are used:

    • _PyFloat_Pack2()
    • _PyFloat_Pack4()
    • _PyFloat_Pack8()
    • _PyFloat_Unpack2()
    • _PyFloat_Unpack4()
    • _PyFloat_Unpack8()

    @vstinner vstinner reopened this Mar 3, 2022
    @vstinner vstinner changed the title Make _PyFloat_(Pack|Unpack)(4|8) cpython API, not internal. Add PyFloat_(Pack|Unpack)(2|4|8) to the public C API Mar 3, 2022
    @vstinner vstinner reopened this Mar 3, 2022
    @vstinner vstinner changed the title Make _PyFloat_(Pack|Unpack)(4|8) cpython API, not internal. Add PyFloat_(Pack|Unpack)(2|4|8) to the public C API Mar 3, 2022
    @vstinner
    Copy link
    Member

    vstinner commented Mar 3, 2022

    The pack/unpack functions have been moved to the internal C API by this change:

    commit 0a883a7
    Author: Victor Stinner <vstinner@python.org>
    Date: Thu Oct 14 23:41:06 2021 +0200

    bpo-35134: Add [Include/cpython/floatobject.h](https://github.com/python/cpython/blob/main/Include/cpython/floatobject.h) (GH-28957)

Split [Include/floatobject.h](https://github.com/python/cpython/blob/main/Include/floatobject.h) into sub-files: add
[Include/cpython/floatobject.h](https://github.com/python/cpython/blob/main/Include/cpython/floatobject.h) and
[Include/internal/pycore_floatobject.h](https://github.com/python/cpython/blob/main/Include/internal/pycore_floatobject.h).

    @vstinner
    Copy link
    Member

    vstinner commented Mar 3, 2022

    I prepared a pythoncapi_compat PR to provide these functions to Python 3.10 and older:
    python/pythoncapi-compat#26

    Note: bpo-11734 (commit 7c4e409) added _PyFloat_Pack2() and _PyFloat_Unpack2() to Python 3.6.0b1.

    @vstinner
    Copy link
    Member

    New changeset 882d809 by Victor Stinner in branch 'main':
    bpo-46906: Add PyFloat_Pack8() to the C API (GH-31657)
    882d809

    @vstinner
    Copy link
    Member

    PR for msgpack: msgpack/msgpack-python#499

    @vstinner
    Copy link
    Member

    PR for bitstruct: eerimoq/bitstruct#26

    @vstinner
    Copy link
    Member

    zodbpickle issue: zopefoundation/zodbpickle#67

    @vstinner
    Copy link
    Member

    New changeset 11c25b8 by Victor Stinner in branch 'main':
    bpo-46906: Mention native endian in PyFloat_Pack8() doc (GH-31866)
    11c25b8

    @vstinner
    Copy link
    Member

    msgpack and bitstruct use the newly added functions: my two PRs got merged. msgpack was my main motivation to add these functions :-)

    Thanks to great reviews, the functions got a new better documentation!

    I close the issue. Thanks again for reviews!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @vstinner vstinner mentioned this issue May 11, 2022
    Closed
    @Yun-Kim Yun-Kim mentioned this issue Aug 25, 2022
    Merged
    14 tasks
    @arunpersaud arunpersaud mentioned this issue Dec 2, 2022
    Closed
    @boisgera boisgera mentioned this issue Sep 4, 2023
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.11 only security fixes topic-C-API
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @methane