← 返回首页
gh-86542: New C-APIs to simplify module attribute declaration by tiran · Pull Request #23286 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

gh-86542: New C-APIs to simplify module attribute declaration#23286

Closed
tiran wants to merge 13 commits into
python:mainfrom
tiran:module_constants
Closed

gh-86542: New C-APIs to simplify module attribute declaration#23286
tiran wants to merge 13 commits into
python:mainfrom
tiran:module_constants

Conversation

tiran commented Nov 14, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  • Check for reference leaks
  • verify refcounts.dat
  • add CAPI tests

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue42376

tiran marked this pull request as draft November 14, 2020 15:45
tiran force-pushed the module_constants branch 3 times, most recently from ee79bab to ef08e94 Compare November 16, 2020 14:36
tiran changed the title [PoC] Define module constants in PyModuleDef bpo-42376: New C-APIs to simplify module attribute declaration Nov 16, 2020
tiran force-pushed the module_constants branch 2 times, most recently from 51767ca to bcd0d45 Compare November 16, 2020 20:56
tiran marked this pull request as ready for review November 17, 2020 07:33
tiran mentioned this pull request Nov 18, 2020
Merged
tiran force-pushed the module_constants branch 2 times, most recently from ed6c1b3 to 6de854c Compare November 22, 2020 13:27
vstinner reviewed Nov 23, 2020
View reviewed changes
Comment thread Include/moduleobject.h Outdated

vstinner Nov 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I'm not sure how to make this member future-proof in term of stable ABI.

We should try to put a max_align_t inside, but this type requires C11. GCC defines it with:

typedef struct { long long __max_align_ll __attribute__((__aligned__(__alignof__(long long)))); long double __max_align_ld __attribute__((__aligned__(__alignof__(long double)))); /* _Float128 is defined as a basic type, so max_align_t must be sufficiently aligned for it. This code must work in C++, so we use __float128 here; that is only available on some architectures, but only on i386 is extra alignment needed for __float128. */ #ifdef __i386__ __float128 __max_align_f128 __attribute__((__aligned__(__alignof(__float128)))); #endif } max_align_t;

Maybe we can at least put long long and long double:

// Unused members added to make PyModuleConst_Def large enough // to get a stable ABI and support future additions. long long m_long_long; long double m_long_double;

encukou Nov 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

This is the only issue I found here :)
The stable ABI doesn't have a good story around evolving structs, and I think we should design a general mechanism for that rather than try to future-proof individual structs.
To move this PR forward, could we exclude PyModule_AddConstants & co. from the limited API for the time being?

Comment thread Include/modsupport.h Outdated

vstinner Nov 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Could you start by adding these two functions in a separated PR, to make this PR shorter?

encukou Nov 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I don't really think that's necessary; the PR is not that big.

tiran mentioned this pull request Nov 23, 2020
Merged
serhiy-storchaka self-requested a review November 23, 2020 10:43
shihai1991 reviewed Nov 26, 2020
View reviewed changes
Comment thread Doc/c-api/module.rst Outdated

shihai1991 Nov 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

nitpick: static PyObject * in here.

Comment thread Modules/posixmodule.c Outdated

shihai1991 Nov 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

How about using another sepeareted PR to do this converation operation?

Comment thread Objects/moduleobject.c Outdated Show resolved Hide resolved

github-actions Bot commented Dec 27, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 27, 2020
tiran force-pushed the module_constants branch from 84759e5 to 3029bb4 Compare March 27, 2021 14:16
tiran added 10 commits April 17, 2021 12:19
Define module constants in PyModuleDef …
436c1b0
Signed-off-by: Christian Heimes <christian@python.org>
Add PyModule_AddTypeFromSpec
d93f2da
PyModule_AddNew* helpers …
a1c8240
Add helpers to create new type/exception and add it to the module dict in one call. Signed-off-by: Christian Heimes <christian@python.org>
Rename new API to PyModuleConst_ …
a277939
Signed-off-by: Christian Heimes <christian@python.org>
Use Py_mod_constants instead of PyModuleDef.m_constants
22e4f7f
Revert "Use Py_mod_constants instead of PyModuleDef.m_constants" …
31f8189
This reverts commit 042e0999ed5ecf88de63de1140d968ab14745e3f.
Remove PyModuleDef.m_constants again
366e810
bpo-42376: New C-APIs to simplify module attribute declaration
bb41d82
ULong, private types, add module to call
751c560
Add CAPI tests
c34a4df
tiran force-pushed the module_constants branch from 3029bb4 to 8458120 Compare April 17, 2021 10:20
encukou reviewed Nov 16, 2021
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

This just needs a bit of polish, and checking the refleaks/refcounts as the OP suggests.
@tiran, do you plan to work on this? Should I take over?

Comment thread Doc/c-api/module.rst
Comment on lines +643 to +659
The values for *type* and the definition of the *value* union are
internal implementation details. Use any of the ``PyModuleConst_`` macros
to define entries. The array must be terminated by an entry with name
set to ``NULL``.

.. c:member:: const char *name

Attribute name.

.. c:member:: int type

Attribute type.

.. c:member:: void *value

Value of the module constant definition, whose meaning depends on
*type*.

encukou Nov 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment
Suggested change
The values for *type* and the definition of the *value* union are
internal implementation details. Use any of the ``PyModuleConst_`` macros
to define entries. The array must be terminated by an entry with name
set to ``NULL``.
.. c:member:: const char *name
Attribute name.
.. c:member:: int type
Attribute type.
.. c:member:: void *value
Value of the module constant definition, whose meaning depends on
*type*.
Definition of a module constant.
The members of this struct are internal implementation details.
To define entries, use only the ``PyModuleConst_`` macros below,
and ``{NULL}`` to mark the end of the array.

Comment thread Python/modsupport.c
}

PyObject *
PyModule_AddNewException(PyObject *module, const char *name, const char *doc,

encukou Nov 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

These two functions need tests.

encukou Dec 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Hm, I see these return borrowed references (held by the module object). That saves a DECREF if you don't need the new object, but it'll be problematic for HPy.

Comment thread Objects/moduleobject.c Outdated Show resolved Hide resolved
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 31, 2022
erlend-aasland changed the title bpo-42376: New C-APIs to simplify module attribute declaration gh-86542: New C-APIs to simplify module attribute declaration Jun 22, 2023
erlend-aasland mentioned this pull request Jan 4, 2024
Closed
erlend-aasland marked this pull request as draft January 4, 2024 11:22
tiran mentioned this pull request May 13, 2023
Closed

encukou commented Jan 11, 2024

Copy link
Copy Markdown
Member

I'll close this as it's not likely to get updated any time soon.
If anyone wants to pick this up, see the issue.

encukou closed this Jan 11, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner left review comments
encukou encukou left review comments
shihai1991 shihai1991 left review comments
serhiy-storchaka Awaiting requested review from serhiy-storchaka
gpshead Awaiting requested review from gpshead gpshead will be requested when the pull request is marked ready for review gpshead is a code owner
picnixz Awaiting requested review from picnixz picnixz will be requested when the pull request is marked ready for review picnixz is a code owner
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently will be requested when the pull request is marked ready for review ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Footer

© 2026 GitHub, Inc.