implement ffi/cpython/pystate #1687
Conversation
src/ffi/pystate.rs
Outdated
|
|
||
| /// Not actually a public field | ||
| /// Also, it has an additional argument in 3.9 | ||
| #[cfg(not(Py_3_9))] |
There was a problem hiding this comment.
I'm not sure that this is a good way to handle this, any suggestions?
There was a problem hiding this comment.
I don't think we should be restricting the ffi::cpython modules to pub(crate). They're already public in the CPython API, so I think it's correct for them to be pub, not pub(crate).
Also, I think that we should probably remove the PyInterpreterState struct as that looks scary - see other comment.
src/ffi/pystate.rs
Outdated
| @@ -7,10 +8,29 @@ use std::os::raw::{c_int, c_long}; | |||
| pub const MAX_CO_EXTRA_USERS: c_int = 255; | |||
|
|
|||
| #[repr(C)] | |||
| #[derive(Copy, Clone)] | |||
| #[derive(Debug, Copy, Clone)] | |||
| pub struct PyInterpreterState { | |||
There was a problem hiding this comment.
Hmm, this looks dangerously broken, because it doesn't match at all the full contents of the corresponding struct in the Python headers: https://github.com/python/cpython/blob/af5fb6706219d7949c1db5c9f2b7da53198123f3/Include/internal/pycore_pystate.h#L67
Appreciate that this existed before you wrote this PR, but I don't think editing this struct in this PR achieves much.
As it's an internal type (and probably very different on PyPy),I think we should just replace with an opaque type:
opaque_struct!(PyInterpreterState);
Their contents would still be public, just not the modules themselves. Currently when you search for something that is defined in the cpython module this is what you see, because it's glob imported twice:
|
👍 I'm persuaded; also the modules in |
|
Happy to merge this once conflict caused by #1692 is resolved (sorry!) |
Per #1675
Also:
pub(crate)to avoid double glob imports (is this OK?)