-
Notifications
You must be signed in to change notification settings - Fork 770
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: methodobject to moduleobject #1425
Conversation
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.
As always these changes look great. Thank you so much for your careful efforts on this cleanup!
pub unsafe fn PyCFunction_CheckExact(op: *mut PyObject) -> c_int { | ||
(Py_TYPE(op) == &mut PyCFunction_Type) as c_int | ||
} | ||
|
||
#[inline] | ||
pub unsafe fn PyCFunction_Check(op: *mut PyObject) -> c_int { | ||
(Py_TYPE(op) == &mut PyCFunction_Type) as c_int |
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 definition PyCFunction_Check
is the same as the new PyCFunction_CheckExact
- does it need updating?
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.
Oops, thanks. Looks like this one was renamed in Python 3.9, with the new definition of PyCFunction_Check
using PyObject_TypeCheck
. Updated.
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.
Looks like 3.9 introduced the Py_IS_TYPE
macro in object.h
, which I'll add -- should help catch these changes when eyeballing
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.
Thanks!
unsafe { self.str_from_ptr(ffi::PyModule_GetFilename(self.as_ptr())) } | ||
unsafe { | ||
self.py() | ||
.from_owned_ptr_or_err::<PyString>(ffi::PyModule_GetFilenameObject(self.as_ptr()))? |
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.
👍🏼
#[deprecated(note = "not present in Python headers; to be removed")] | ||
pub const PyModuleDef_INIT: PyModuleDef = PyModuleDef { |
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 would be breaking, but I guess the only user is pyclass.rs
and we can remove this.
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.
Agreed, as I think we're going to do a breaking release next anyway it's fine to just scrap this.
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.
@davidhewitt You'll do this in #1426 or should I add to this PR?
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 think probably simpler for you to do it in this PR so there's no risk of merge conflicts.
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.
Actually nvm, I see that other than that, this PR is ready to go. I'll merge this PR now and do it in #1426.
👍 Thanks again! |
Main changes are to deprecate some functions to reflect the C API. This one isn't too challenging either, but next is
object.h
, which will require some major refactoring tocpython/
.This PR reflects
PyModule_GetFilename
(returning achar*
) being deprecated in Python 3.2 in favour ofPyModule_GetFilenameObject
(returning aPyObject
). We do use the former, but for now I've just put an#[allow(deprecated)]
on that line.