Skip to content

Upgrade CPython to the new PyMemberDef API: names starting with Py_ prefix (Py_T_INT instead of T_INT) #106869

Closed
Closed
Upgrade CPython to the new PyMemberDef API: names starting with Py_ prefix (Py_T_INT instead of T_INT)#106869
Assignees
vstinner
Labels
type-featureA feature request or enhancement

Description

@vstinner
vstinner
opened on Jul 18, 2023

The CPython code base was written for the old "structmember.h" API with names like READONLY and T_INT. I propose to upgrade the code base to new public names like Py_READONLY and Py_T_INT.

#include "structmember.h" should be removed, but #include <stddef.h> should maybe be added in exchange to get the standard offsetof() function.

Linked PRs

Activity

  1. vstinner
    added
    type-featureA feature request or enhancement
    on Jul 18, 2023

  2. vstinner commented on Jul 18, 2023

    @vstinner
    vstinner
    on Jul 18, 2023
    MemberAuthor

    I don't get why 3 constants are private, name starting with _Py prefix:

    By the way, (Py_AUDIT_READ | _Py_WRITE_RESTRICTED) replaces RESTRICTED, it uses _Py_WRITE_RESTRICTED.

  3. vstinner commented on Jul 18, 2023

    @vstinner
    vstinner
    on Jul 18, 2023
    MemberAuthor

    _Py_WRITE_RESTRICTED and its deprecated alias WRITE_RESTRICTED do nothing. Maybe their usage should just be removed in the code.

  4. vstinner
    added a commit that references this issue on Jul 18, 2023

    pythongh-106869: Use new PyMemberDef constant names

    0ff7029

  5. vstinner commented on Jul 18, 2023

    @vstinner
    vstinner
    on Jul 18, 2023
    MemberAuthor

    I generated a big PR making all mechanical name changes at once: PR #106871. Maybe this PR is too big (ex: to be reviewed), I can slice it into smaller PRs (ex: 20 files max per PR).

    I created this PR to see how it would look, so we can agree on how the change should be done.

    cc @encukou @serhiy-storchaka

  6. vstinner
    added 4 commits that reference this issue on Jul 25, 2023

    pythongh-106869: Use new PyMemberDef constant names

    11bef5d

    pythongh-106869: Use new PyMemberDef constant names

    0c91ecc

    pythongh-106869: Use new PyMemberDef constant names

    8d13210

    gh-106869: Use new PyMemberDef constant names (#106871)

    1a3faba

  7. vstinner commented on Jul 25, 2023

    @vstinner
    vstinner
    on Jul 25, 2023
    MemberAuthor

    Ok, I updated 78 files to use the new public API rather than "structmember.h" API which doesn't use the Py_ prefix.

    Thanks @encukou for the new API :-)

    About questions on the new API: well, they can be addressed separately.

    For now, I replaced T_OBJECT with _Py_T_OBJECT. Maybe some files should be updated to use Py_T_OBJECT_EX and so raise AttributeError if the attribute was deleted (set to None), rather than returning None.

  8. vstinner
    closed this as completedon Jul 25, 2023

  9. serhiy-storchaka commented on Jul 25, 2023

    @serhiy-storchaka
    serhiy-storchaka
    on Jul 25, 2023
    Member

    Maybe some files should be updated to use Py_T_OBJECT_EX and so raise AttributeError if the attribute was deleted (set to None), rather than returning None.

    I am doing this right now. 😉

    The main effect not in deletion, but that these attributes are None by default.

  10. jtcave
    added a commit that references this issue on Jul 27, 2023

    pythongh-106869: Use new PyMemberDef constant names (python#106871)

    0f2adf1

  11. encukou commented on Aug 2, 2023

    @encukou
    encukou
    on Aug 2, 2023
    Member

    Usage of the old names meant that they were tested, however indirectly. Now the old names have no tests. Could you add tests?

  12. encukou
    reopened this on Aug 2, 2023
  13. encukou
    assigned
    vstinner
    on Aug 2, 2023

  14. encukou commented on Aug 2, 2023

    @encukou
    encukou
    on Aug 2, 2023
    Member

    I don't get why 3 constants are private, name starting with _Py prefix: [_Py_T_OBJECT, _Py_T_NONE, _Py_WRITE_RESTRICTED]

    They should not be used in new code. Existing code can continue to use structmember.h and the original names.

  15. vstinner commented on Aug 26, 2023

    @vstinner
    vstinner
    on Aug 26, 2023
    MemberAuthor

    @encukou reopened this Aug 2, 2023

    Why did you reopen this issue?

    They should not be used in new code. Existing code can continue to use structmember.h and the original names.

    Ok.

  16. encukou commented on Aug 31, 2023

    @encukou
    encukou
    on Aug 31, 2023
    Member

    Ah, this is the PR about which I messaged you privately. Thanks for finding it!
    The tests are there, in Modules/_testcapi/structmember.c, so this can remain closed.

  17. encukou
    closed this as completedon Aug 31, 2023

  18. vstinner commented on Sep 1, 2023

    @vstinner
    vstinner
    on Sep 1, 2023
    MemberAuthor

    The tests are there, in Modules/_testcapi/structmember.c, so this can remain closed.

    I modified this file twice by mistake, but hopefully, I noticed it and the final merged PR leaves it unchanged.

    Part 1, new API:

    static struct PyMemberDef test_members_newapi[] = {
    {"T_BOOL", Py_T_BOOL, offsetof(test_structmembers, structmembers.bool_member), 0, NULL},
    {"T_BYTE", Py_T_BYTE, offsetof(test_structmembers, structmembers.byte_member), 0, NULL},
    {"T_UBYTE", Py_T_UBYTE, offsetof(test_structmembers, structmembers.ubyte_member), 0, NULL},
    {"T_SHORT", Py_T_SHORT, offsetof(test_structmembers, structmembers.short_member), 0, NULL},
    {"T_USHORT", Py_T_USHORT, offsetof(test_structmembers, structmembers.ushort_member), 0, NULL},
    {"T_INT", Py_T_INT, offsetof(test_structmembers, structmembers.int_member), 0, NULL},
    {"T_UINT", Py_T_UINT, offsetof(test_structmembers, structmembers.uint_member), 0, NULL},
    {"T_LONG", Py_T_LONG, offsetof(test_structmembers, structmembers.long_member), 0, NULL},
    {"T_ULONG", Py_T_ULONG, offsetof(test_structmembers, structmembers.ulong_member), 0, NULL},
    {"T_PYSSIZET", Py_T_PYSSIZET, offsetof(test_structmembers, structmembers.pyssizet_member), 0, NULL},
    {"T_FLOAT", Py_T_FLOAT, offsetof(test_structmembers, structmembers.float_member), 0, NULL},
    {"T_DOUBLE", Py_T_DOUBLE, offsetof(test_structmembers, structmembers.double_member), 0, NULL},
    {"T_STRING_INPLACE", Py_T_STRING_INPLACE, offsetof(test_structmembers, structmembers.inplace_member), 0, NULL},
    {"T_LONGLONG", Py_T_LONGLONG, offsetof(test_structmembers, structmembers.longlong_member), 0, NULL},
    {"T_ULONGLONG", Py_T_ULONGLONG, offsetof(test_structmembers, structmembers.ulonglong_member), 0, NULL},
    {NULL}
};

    Part 2, old API:

    static struct PyMemberDef test_members[] = {
    {"T_BOOL", T_BOOL, offsetof(test_structmembers, structmembers.bool_member), 0, NULL},
    {"T_BYTE", T_BYTE, offsetof(test_structmembers, structmembers.byte_member), 0, NULL},
    {"T_UBYTE", T_UBYTE, offsetof(test_structmembers, structmembers.ubyte_member), 0, NULL},
    {"T_SHORT", T_SHORT, offsetof(test_structmembers, structmembers.short_member), 0, NULL},
    {"T_USHORT", T_USHORT, offsetof(test_structmembers, structmembers.ushort_member), 0, NULL},
    {"T_INT", T_INT, offsetof(test_structmembers, structmembers.int_member), 0, NULL},
    {"T_UINT", T_UINT, offsetof(test_structmembers, structmembers.uint_member), 0, NULL},
    {"T_LONG", T_LONG, offsetof(test_structmembers, structmembers.long_member), 0, NULL},
    {"T_ULONG", T_ULONG, offsetof(test_structmembers, structmembers.ulong_member), 0, NULL},
    {"T_PYSSIZET", T_PYSSIZET, offsetof(test_structmembers, structmembers.pyssizet_member), 0, NULL},
    {"T_FLOAT", T_FLOAT, offsetof(test_structmembers, structmembers.float_member), 0, NULL},
    {"T_DOUBLE", T_DOUBLE, offsetof(test_structmembers, structmembers.double_member), 0, NULL},
    {"T_STRING_INPLACE", T_STRING_INPLACE, offsetof(test_structmembers, structmembers.inplace_member), 0, NULL},
    {"T_LONGLONG", T_LONGLONG, offsetof(test_structmembers, structmembers.longlong_member), 0, NULL},
    {"T_ULONGLONG", T_ULONGLONG, offsetof(test_structmembers, structmembers.ulonglong_member), 0, NULL},
    {NULL}
};

    So it's ok, the new and the old APIs are tested, as expected.

    It's good that you also checked :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

type-featureA feature request or enhancement

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions