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

ffi: cleanup pystate #1787

Merged
merged 1 commit into from
Aug 13, 2021
Merged

ffi: cleanup pystate #1787

merged 1 commit into from
Aug 13, 2021

Conversation

davidhewitt
Copy link
Member

  • moves PyGILState_Check and Py_tracefunc to unlimited api
  • makes PyThreadState an opaque struct (the current definition was totally broken, so haven't bothered mentioning this in the CHANGELOG as nobody can have been using it successfully).

@davidhewitt davidhewitt requested a review from mejrs August 12, 2021 07:06
@davidhewitt davidhewitt self-assigned this Aug 12, 2021
@davidhewitt davidhewitt force-pushed the ffi-pystate branch 2 times, most recently from d61b5c4 to 308d76c Compare August 12, 2021 07:49
extern "C" {
#[cfg(not(PyPy))]
#[cfg_attr(docsrs, doc(not(PyPy)))]
Copy link
Member

@mejrs mejrs Aug 12, 2021

Choose a reason for hiding this comment

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

Suggested change
#[cfg_attr(docsrs, doc(not(PyPy)))]
#[cfg_attr(docsrs, doc(cfg(not(PyPy))))]

Same for all the other ones.

Maybe this is worth putting on top of the extern "C" blocks (splitting them if necessary) to reduce littering attributes everywhere?

Note that doc_cfg doesn't work with glob imports, so documenting everything in ffi with these is going to be a huge pain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. For now I'm going to keep it like this, but I agree it would be nice to find a long-term solution.

// skipped _PyErr_StackItem
// skipped _PyStackChunk
// skipped _ts (aka PyThreadState)

extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

The cfgs in this entire block don't do anything because the entire module import is already gated with #[cfg(all(Py_3_8, not(PyPy)))] (https://github.com/PyO3/pyo3/blob/main/src/ffi/cpython/mod.rs#L43).

Copy link
Member Author

Choose a reason for hiding this comment

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

As above will remove that gating and make the cfgs here meaningful.


// skipped _PyInterpreterState_GetMainModule

pub type Py_tracefunc = extern "C" fn(
Copy link
Member

Choose a reason for hiding this comment

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

These functions were in Include/pystate before 3.8. The problem with moving this here now is that the cpython/pystate module is gated behind Py_3_8, so this removes it from 3.6 and 3.7, which is why CI is now failing. (the same thing may be true for other things)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. I'd rather have the ffi modules look as much like the Python headers as possible, to minimise confusion. So I'll remove that version gating and tweak things.

@davidhewitt davidhewitt merged commit 874e7e5 into PyO3:main Aug 13, 2021
@davidhewitt davidhewitt deleted the ffi-pystate branch August 13, 2021 11:36
Sign up for free to 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.

2 participants