From 6f03a5464f53a7f76e406eb7d09610324cd8667c Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:15:35 +0100 Subject: [PATCH] cleans up `PyCFunction::internal_new` (#3910) This deduplicates some code around `PyCFunction::internal_new` --- pyo3-macros-backend/src/pyfunction.rs | 2 +- src/impl_/pyfunction.rs | 17 ++++++----- src/types/function.rs | 43 ++++++--------------------- 3 files changed, 19 insertions(+), 43 deletions(-) diff --git a/pyo3-macros-backend/src/pyfunction.rs b/pyo3-macros-backend/src/pyfunction.rs index 265a4824109..45de94e4a10 100644 --- a/pyo3-macros-backend/src/pyfunction.rs +++ b/pyo3-macros-backend/src/pyfunction.rs @@ -273,7 +273,7 @@ pub fn impl_wrap_pyfunction( pub fn add_to_module(module: &#krate::Bound<'_, #krate::types::PyModule>) -> #krate::PyResult<()> { use #krate::prelude::PyModuleMethods; use ::std::convert::Into; - module.add_function(#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?) + module.add_function(#krate::types::PyCFunction::internal_new(module.py(), &DEF, module.into())?) } } diff --git a/src/impl_/pyfunction.rs b/src/impl_/pyfunction.rs index 531e0c93192..cb838fea6c2 100644 --- a/src/impl_/pyfunction.rs +++ b/src/impl_/pyfunction.rs @@ -1,6 +1,6 @@ use crate::{ types::{PyCFunction, PyModule}, - Borrowed, Bound, PyResult, Python, + Borrowed, Bound, PyNativeType, PyResult, Python, }; pub use crate::impl_::pymethods::PyMethodDef; @@ -13,25 +13,25 @@ pub trait WrapPyFunctionArg<'py, T> { impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Bound<'py, PyModule> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new_bound(self.py(), method_def, Some(&self)) + PyCFunction::internal_new(self.py(), method_def, Some(&self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Bound<'py, PyModule> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new_bound(self.py(), method_def, Some(self)) + PyCFunction::internal_new(self.py(), method_def, Some(self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Borrowed<'_, 'py, PyModule> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new_bound(self.py(), method_def, Some(&self)) + PyCFunction::internal_new(self.py(), method_def, Some(&self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Borrowed<'_, 'py, PyModule> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new_bound(self.py(), method_def, Some(self)) + PyCFunction::internal_new(self.py(), method_def, Some(self)) } } @@ -39,13 +39,14 @@ impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Borrowed<'_, ' // The `wrap_pyfunction_bound!` macro is needed for the Bound form. impl<'py> WrapPyFunctionArg<'py, &'py PyCFunction> for Python<'py> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<&'py PyCFunction> { - PyCFunction::internal_new(method_def, self.into()).map(Bound::into_gil_ref) + PyCFunction::internal_new(self, method_def, None).map(Bound::into_gil_ref) } } impl<'py> WrapPyFunctionArg<'py, &'py PyCFunction> for &'py PyModule { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<&'py PyCFunction> { - PyCFunction::internal_new(method_def, self.into()).map(Bound::into_gil_ref) + PyCFunction::internal_new(self.py(), method_def, Some(&self.as_borrowed())) + .map(Bound::into_gil_ref) } } @@ -63,6 +64,6 @@ where impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for OnlyBound> { fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new_bound(self.0, method_def, None) + PyCFunction::internal_new(self.0, method_def, None) } } diff --git a/src/types/function.rs b/src/types/function.rs index 75173d2aadb..ea8201fb131 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -9,7 +9,7 @@ use crate::{ impl_::pymethods::{self, PyMethodDef}, types::{PyCapsule, PyDict, PyModule, PyString, PyTuple}, }; -use crate::{Bound, IntoPy, Py, PyAny, PyResult, Python}; +use crate::{Bound, IntoPy, Py, PyAny, PyNativeType, PyResult, Python}; use std::cell::UnsafeCell; use std::ffi::CStr; @@ -34,13 +34,15 @@ impl PyCFunction { doc: &'static str, py_or_module: PyFunctionArguments<'a>, ) -> PyResult<&'a Self> { + let (py, module) = py_or_module.into_py_and_maybe_module(); Self::internal_new( + py, &PyMethodDef::cfunction_with_keywords( name, pymethods::PyCFunctionWithKeywords(fun), doc, ), - py_or_module, + module.map(PyNativeType::as_borrowed).as_deref(), ) .map(Bound::into_gil_ref) } @@ -53,7 +55,7 @@ impl PyCFunction { doc: &'static str, module: Option<&Bound<'py, PyModule>>, ) -> PyResult> { - Self::internal_new_bound( + Self::internal_new( py, &PyMethodDef::cfunction_with_keywords( name, @@ -78,9 +80,11 @@ impl PyCFunction { doc: &'static str, py_or_module: PyFunctionArguments<'a>, ) -> PyResult<&'a Self> { + let (py, module) = py_or_module.into_py_and_maybe_module(); Self::internal_new( + py, &PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc), - py_or_module, + module.map(PyNativeType::as_borrowed).as_deref(), ) .map(Bound::into_gil_ref) } @@ -93,7 +97,7 @@ impl PyCFunction { doc: &'static str, module: Option<&Bound<'py, PyModule>>, ) -> PyResult> { - Self::internal_new_bound( + Self::internal_new( py, &PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc), module, @@ -180,35 +184,6 @@ impl PyCFunction { #[doc(hidden)] pub fn internal_new<'py>( - method_def: &PyMethodDef, - py_or_module: PyFunctionArguments<'py>, - ) -> PyResult> { - let (py, module) = py_or_module.into_py_and_maybe_module(); - let (mod_ptr, module_name): (_, Option>) = if let Some(m) = module { - let mod_ptr = m.as_ptr(); - (mod_ptr, Some(m.name()?.into_py(py))) - } else { - (std::ptr::null_mut(), None) - }; - let (def, destructor) = method_def.as_method_def()?; - - // FIXME: stop leaking the def and destructor - let def = Box::into_raw(Box::new(def)); - std::mem::forget(destructor); - - let module_name_ptr = module_name - .as_ref() - .map_or(std::ptr::null_mut(), Py::as_ptr); - - unsafe { - ffi::PyCFunction_NewEx(def, mod_ptr, module_name_ptr) - .assume_owned_or_err(py) - .downcast_into_unchecked() - } - } - - #[doc(hidden)] - pub(crate) fn internal_new_bound<'py>( py: Python<'py>, method_def: &PyMethodDef, module: Option<&Bound<'py, PyModule>>,