[PATCH 7 of 7] rust-cpython: leverage upstreamed py_capsule_fn!() macro
Georges Racinet
georges.racinet at octobus.net
Sun Oct 13 08:08:24 EDT 2019
Hi,
I don't have time for a formal review right now, but I did skim over the
series, and I think it can be simplified and pushed further, removing
the whole sys_python dependency (see the inline comment below)
Actually I have a series of my own in the works to the same effect,
i.e., leveraging the rust-cpython 0.3.0 and its more integrated) capsule
integration. Here it is for reference:
https://dev.heptapod.net/octobus/mercurial-devel/merge_requests/1 (it
also makes use of the new high-level `PySet`, which I can submit
independently).
I did not submit it yet, because I've been slow on rebasing / testing
against Py2 and Py3 with consistent results. You shot first :-) Note
that I don't care much which version lands at the end of the day.
Regards,
On 10/13/19 11:47 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya at tcha.org>
> # Date 1570954064 -32400
> # Sun Oct 13 17:07:44 2019 +0900
> # Node ID 6c8410bd4ce786ceca53986514a1fbda5f097f2d
> # Parent 2d7671cc1d717f3ed3890e47324e4ed479529466
> rust-cpython: leverage upstreamed py_capsule_fn!() macro
Side remark: at some point I'd like to switch the Index capsule to the
array-of-functions-and-constants style, as in
https://dgrunwald.github.io/rust-cpython/doc/cpython/struct.PyCapsule.html#using-a-capsule-defined-in-another-extension-module
This is how all capsules defined in the Python standard library actually
work.
>
> diff --git a/rust/hg-cpython/src/cindex.rs b/rust/hg-cpython/src/cindex.rs
> --- a/rust/hg-cpython/src/cindex.rs
> +++ b/rust/hg-cpython/src/cindex.rs
> @@ -10,18 +10,20 @@
> //! Ideally, we should use an Index entirely implemented in Rust,
> //! but this will take some time to get there.
>
> -use crate::python_sys::{self, PyCapsule_Import};
> -use cpython::{PyClone, PyErr, PyObject, PyResult, Python};
> +use crate::python_sys;
> +use cpython::{PyClone, PyObject, PyResult, Python};
> use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
> use libc::c_int;
> -use std::ffi::CStr;
> -use std::mem::transmute;
>
> -type IndexParentsFn = unsafe extern "C" fn(
> - index: *mut python_sys::PyObject,
> - rev: c_int,
> - ps: *mut [c_int; 2],
> -) -> c_int;
> +py_capsule_fn!(
> + from mercurial.cext.parsers import index_get_parents_CAPI
> + as get_parents_capi
> + signature (
> + index: *mut python_sys::PyObject,
Here you can directly use RawPyObject:
index: *mut RawPyObject,
see
https://github.com/dgrunwald/rust-cpython/commit/c5cfd32cf05005e3ad515063c9ca14100362eb93
the same would apply for other capsules, and that means we can
completely get rid of the dependency to sys_python
> + rev: c_int,
> + ps: *mut [c_int; 2],
> + ) -> c_int
> +);
>
> /// A `Graph` backed up by objects and functions from revlog.c
> ///
> @@ -57,14 +59,14 @@ type IndexParentsFn = unsafe extern "C"
> /// mechanisms in other contexts.
> pub struct Index {
> index: PyObject,
> - parents: IndexParentsFn,
> + parents: get_parents_capi::CapsuleFn,
> }
>
> impl Index {
> pub fn new(py: Python, index: PyObject) -> PyResult<Self> {
> Ok(Index {
> index: index,
> - parents: decapsule_parents_fn(py)?,
> + parents: get_parents_capi::retrieve(py)?,
> })
> }
> }
> @@ -99,31 +101,3 @@ impl Graph for Index {
> }
> }
> }
> -
> -/// Return the `index_get_parents` function of the parsers C Extension module.
> -///
> -/// A pointer to the function is stored in the `parsers` module as a
> -/// standard [Python capsule](https://docs.python.org/2/c-api/capsule.html).
> -///
> -/// This function retrieves the capsule and casts the function pointer
> -///
> -/// Casting function pointers is one of the rare cases of
> -/// legitimate use cases of `mem::transmute()` (see
> -/// https://doc.rust-lang.org/std/mem/fn.transmute.html of
> -/// `mem::transmute()`.
> -/// It is inappropriate for architectures where
> -/// function and data pointer sizes differ (so-called "Harvard
> -/// architectures"), but these are nowadays mostly DSPs
> -/// and microcontrollers, hence out of our scope.
> -fn decapsule_parents_fn(py: Python) -> PyResult<IndexParentsFn> {
> - unsafe {
> - let caps_name = CStr::from_bytes_with_nul_unchecked(
> - b"mercurial.cext.parsers.index_get_parents_CAPI\0",
> - );
> - let from_caps = PyCapsule_Import(caps_name.as_ptr(), 0);
> - if from_caps.is_null() {
> - return Err(PyErr::fetch(py));
> - }
> - Ok(transmute(from_caps))
> - }
> -}
> diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
> --- a/rust/hg-cpython/src/dirstate.rs
> +++ b/rust/hg-cpython/src/dirstate.rs
> @@ -15,7 +15,7 @@ mod dirs_multiset;
> mod dirstate_map;
>
> use crate::dirstate::{dirs_multiset::Dirs, dirstate_map::DirstateMap};
> -use crate::python_sys::{self, PyCapsule_Import};
> +use crate::python_sys;
> use cpython::{
> exc, PyBytes, PyDict, PyErr, PyModule, PyObject, PyResult, PySequence,
> Python,
> @@ -26,8 +26,6 @@ use hg::{
> };
> use libc::{c_char, c_int};
> use std::convert::TryFrom;
> -use std::ffi::CStr;
> -use std::mem::transmute;
>
> // C code uses a custom `dirstate_tuple` type, checks in multiple instances
> // for this type, and raises a Python `Exception` if the check does not pass.
> @@ -35,34 +33,23 @@ use std::mem::transmute;
> // would be a good idea in the near future to remove it entirely to allow
> // for a pure Python tuple of the same effective structure to be used,
> // rendering this type and the capsule below useless.
> -type MakeDirstateTupleFn = unsafe extern "C" fn(
> - state: c_char,
> - mode: c_int,
> - size: c_int,
> - mtime: c_int,
> -) -> *mut python_sys::PyObject;
> -
> -// This is largely a copy/paste from cindex.rs, pending the merge of a
> -// `py_capsule_fn!` macro in the rust-cpython project:
> -// https://github.com/dgrunwald/rust-cpython/pull/169
> -fn decapsule_make_dirstate_tuple(py: Python) -> PyResult<MakeDirstateTupleFn> {
> - unsafe {
> - let caps_name = CStr::from_bytes_with_nul_unchecked(
> - b"mercurial.cext.parsers.make_dirstate_tuple_CAPI\0",
> - );
> - let from_caps = PyCapsule_Import(caps_name.as_ptr(), 0);
> - if from_caps.is_null() {
> - return Err(PyErr::fetch(py));
> - }
> - Ok(transmute(from_caps))
> - }
> -}
> +py_capsule_fn!(
> + from mercurial.cext.parsers import make_dirstate_tuple_CAPI
> + as make_dirstate_tuple_capi
> + signature (
> + state: c_char,
> + mode: c_int,
> + size: c_int,
> + mtime: c_int,
> + ) -> *mut python_sys::PyObject
> +);
>
> pub fn make_dirstate_tuple(
> py: Python,
> entry: &DirstateEntry,
> ) -> PyResult<PyObject> {
> - let make = decapsule_make_dirstate_tuple(py)?;
> + // might be silly to retrieve capsule function in hot loop
> + let make = make_dirstate_tuple_capi::retrieve(py)?;
>
> let &DirstateEntry {
> state,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
--
Georges Racinet
https://octobus.net
GPG: BF5456F4DC625443849B6E58EE20CA44EF691D39, sur serveurs publics
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20191013/f071047f/attachment.sig>
More information about the Mercurial-devel
mailing list