GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-86542: New C-APIs to simplify module attribute declaration by tiran · Pull Request #23286 · python/cpython

Conversation

@tiran

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

Uh oh!

There was an error while loading. Please reload this page.

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

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue42376

  • Issue: Add helpers to populate modules in C #86542

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 14, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Nov 14, 2020
@tiran
tiran marked this pull request as draft November 14, 2020 15:45
@tiran
tiran force-pushed the module_constants branch 3 times, most recently from ee79bab to ef08e94 Compare November 16, 2020 14:36
@tiran tiran changed the title [PoC] Define module constants in PyModuleDef bpo-42376: New C-APIs to simplify module attribute declaration Nov 16, 2020
@tiran
tiran force-pushed the module_constants branch 2 times, most recently from 51767ca to bcd0d45 Compare November 16, 2020 20:56
@tiran
tiran marked this pull request as ready for review November 17, 2020 07:33
@tiran tiran mentioned this pull request Nov 18, 2020
bpo-1635741: Port symtable module to multiphase initialization #23361
Merged
@tiran
tiran force-pushed the module_constants branch 2 times, most recently from ed6c1b3 to 6de854c Compare November 22, 2020 13:27
vstinner
vstinner reviewed Nov 23, 2020
View reviewed changes
Comment thread Include/moduleobject.h Outdated

@vstinner vstinner Nov 23, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@encukou encukou Nov 24, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/modsupport.h Outdated

@vstinner vstinner Nov 23, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@encukou encukou Nov 24, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tiran tiran mentioned this pull request Nov 23, 2020
bpo-42327: Add PyModule_Add() (smaller). #23443
Merged
@serhiy-storchaka
serhiy-storchaka self-requested a review November 23, 2020 10:43
shihai1991
shihai1991 reviewed Nov 26, 2020
View reviewed changes
Comment thread Doc/c-api/module.rst Outdated

@shihai1991 shihai1991 Nov 26, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nitpick: static PyObject * in here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/posixmodule.c Outdated

@shihai1991 shihai1991 Nov 26, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 27, 2020
@tiran
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
@tiran
Define module constants in PyModuleDef
436c1b0 
Signed-off-by: Christian Heimes <[email protected]>
@tiran
Add PyModule_AddTypeFromSpec
d93f2da
@tiran
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 <[email protected]>
@tiran
Rename new API to PyModuleConst_
a277939 
Signed-off-by: Christian Heimes <[email protected]>
@tiran
Use Py_mod_constants instead of PyModuleDef.m_constants
22e4f7f
@tiran
Revert "Use Py_mod_constants instead of PyModuleDef.m_constants"
31f8189 
This reverts commit 042e0999ed5ecf88de63de1140d968ab14745e3f.
@tiran
Remove PyModuleDef.m_constants again
366e810
@tiran
bpo-42376: New C-APIs to simplify module attribute declaration
bb41d82
@tiran
ULong, private types, add module to call
751c560
@tiran
Add CAPI tests
c34a4df
tiran added 3 commits April 17, 2021 12:19
@tiran
Typo
3e50f44
@tiran
Use single base class feature from bpo-42423
00236fe
@tiran
It's a strong ref
8458120
@tiran
tiran force-pushed the module_constants branch from 3029bb4 to 8458120 Compare April 17, 2021 10:20
encukou
encukou reviewed Nov 16, 2021
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 encukou Nov 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/modsupport.c
}

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

@encukou encukou Nov 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

These two functions need tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@encukou encukou Dec 14, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 31, 2022
@erlend-aasland 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 erlend-aasland mentioned this pull request Jan 4, 2024
C API for simplifying module attribute declaration capi-workgroup/decisions#6
Closed
@erlend-aasland
erlend-aasland marked this pull request as draft January 4, 2024 11:22
@bedevere-app bedevere-app Bot removed the awaiting core review label Jan 4, 2024
@tiran tiran mentioned this pull request May 13, 2023
Add helpers to populate modules in C #86542
Closed
@encukou

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.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@encukou encukou encukou left review comments
@shihai1991 shihai1991 shihai1991 left review comments
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
@gpshead 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 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 ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently will be requested when the pull request is marked ready for review ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@tiran @encukou @vstinner @shihai1991 @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.