ffi module cleanup #1338
ffi module cleanup #1338
Conversation
| use crate::ffi::object::PyObject; | ||
| use crate::ffi::pystate::{PyThreadState, Py_tracefunc}; | ||
| use crate::ffi::pystate::PyThreadState; | ||
| use std::os::raw::{c_char, c_int, c_void}; | ||
|
|
||
| extern "C" { |
There was a problem hiding this comment.
Can't select the exact line, but PyEval_CallObjectWithKeywords is marked deprecated in 3.9. However, if I mark it with #[cfg_attr(Py_3_9, deprecated(note = "Python 3.9"))], I can't compile due to deny(warnings) on this crate (it's used by PyEval_CallObject below).
Maybe we could consider being a bit more specific with the crate's deny attribute? https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deny-warnings.md
There was a problem hiding this comment.
Thanks, I recently added this and I agree it's now an antipattern.
I've opened #1340 to remove the attribute.
As specifically here, I think we will need to put #[allow(deprecated)] on the call inside PyEval_CallObject so that this passes CI. Mildly annoying, but hey!
There was a problem hiding this comment.
This is fine, I think. I think the only gotcha is that we need to add the deprecation to PyEval_CallObject as well.
src/ffi/code.rs
Outdated
| crate::ffi::tupleobject::PyTuple_GET_SIZE((*op).co_freevars) | ||
| } | ||
| // TODO: newtype | ||
| pub use crate::ffi::PyCodeObject; |
There was a problem hiding this comment.
The definition in Include/code.h is typedef struct PyCodeObject PyCodeObject, where the actual def is in Include/cpython/code.h.
I plan to change this into a newtype, but this requires some other code changes I'm not prepared to make yet. Just making a note here.
There was a problem hiding this comment.
I think what we need is
#[cfg(Py_LIMITED_API)]
opaque_struct!(PyCodeObject);Also, we have to remove #[cfg(not(Py_LIMITED_API)] from mod code;.
There was a problem hiding this comment.
Also, let's remove pub use crate::ffi::PyCodeObject;.
Since we import all items in mod.rs, this is not so mearningful.
|
Hmm, I'm not sure what to make of (from failing python3.6-x86 windows-latest). Is this something you know about or do I need to try smaller changes? |
src/ffi/code.rs
Outdated
| crate::ffi::tupleobject::PyTuple_GET_SIZE((*op).co_freevars) | ||
| } | ||
| // TODO: newtype | ||
| pub use crate::ffi::PyCodeObject; |
There was a problem hiding this comment.
I think what we need is
#[cfg(Py_LIMITED_API)]
opaque_struct!(PyCodeObject);Also, we have to remove #[cfg(not(Py_LIMITED_API)] from mod code;.
| use crate::ffi::object::PyObject; | ||
| use crate::ffi::pystate::{PyThreadState, Py_tracefunc}; | ||
| use crate::ffi::pystate::PyThreadState; | ||
| use std::os::raw::{c_char, c_int, c_void}; | ||
|
|
||
| extern "C" { |
There was a problem hiding this comment.
Thanks, I recently added this and I agree it's now an antipattern.
I've opened #1340 to remove the attribute.
As specifically here, I think we will need to put #[allow(deprecated)] on the call inside PyEval_CallObject so that this passes CI. Mildly annoying, but hey!
|
The Also, if you are willing to add a CHANGELOG entry documented the new and deprecated APIs, that'd be excellent 🙏 |
Co-authored-by: Yuji Kanagawa <[email protected]>
|
All done, I think. Added changelog entry for deprecations (not adding any functions yet). Thanks! |
|
Thanks! |
Partial fix for #1289, as suggested in #1289 (comment)