Skip to content

Conversation

@tiran
Copy link
Member

@tirantiran commented Nov 14, 2020

  • Check for reference leaks
  • verify refcounts.dat
  • add CAPI tests

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

https://bugs.python.org/issue42376

@tirantiran marked this pull request as draft November 14, 2020 15:45
@tirantiranforce-pushed the module_constants branch 3 times, most recently from ee79bab to ef08e94CompareNovember 16, 2020 14:36
@tirantiran changed the title [PoC] Define module constants in PyModuleDefbpo-42376: New C-APIs to simplify module attribute declarationNov 16, 2020
@tirantiranforce-pushed the module_constants branch 2 times, most recently from 51767ca to bcd0d45CompareNovember 16, 2020 20:56
@tirantiran marked this pull request as ready for review November 17, 2020 07:33
@tirantiranforce-pushed the module_constants branch 2 times, most recently from ed6c1b3 to 6de854cCompareNovember 22, 2020 13:27
Copy link
Member

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; 

Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

@github-actions
Copy link

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

@github-actionsgithub-actionsbot added the stale Stale PR or inactive for long period of time. label Dec 27, 2020
tiran added 10 commits April 17, 2021 12:19
Signed-off-by: Christian Heimes <christian@python.org>
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>
Signed-off-by: Christian Heimes <christian@python.org>
This reverts commit 042e0999ed5ecf88de63de1140d968ab14745e3f.
Copy link
Member

@encukouencukou left a 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?

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::constchar *name
Attribute name.
.. c:member::int type
Attribute type.
.. c:member::void *value
Value of the module constant definition, whose meaning depends on
*type*.
Copy link
Member

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::constchar *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.

}

PyObject*
PyModule_AddNewException(PyObject*module, constchar*name, constchar*doc,
Copy link
Member

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.

Copy link
Member

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.

@github-actionsgithub-actionsbot removed the stale Stale PR or inactive for long period of time. label Jul 31, 2022
@erlend-aaslanderlend-aasland changed the title bpo-42376: New C-APIs to simplify module attribute declarationgh-86542: New C-APIs to simplify module attribute declarationJun 22, 2023
@erlend-aaslanderlend-aasland marked this pull request as draft January 4, 2024 11:22
@encukou
Copy link
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.

@encukouencukou closed this Jan 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@tiran@encukou@vstinner@shihai1991@the-knights-who-say-ni@ezio-melotti@bedevere-bot