Patchwork [7,of,7] rust-cpython: leverage upstreamed py_capsule_fn!() macro

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 13, 2019, 9:47 a.m.
Message ID <6c8410bd4ce786ceca53.1570960030@mimosa>
Download mbox | patch
Permalink /patch/42293/
State New
Headers show

Comments

Yuya Nishihara - Oct. 13, 2019, 9:47 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@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
Georges Racinet - Oct. 13, 2019, 12:08 p.m.
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@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 13, 2019, 12:37 p.m.
On Sun, 13 Oct 2019 14:08:24 +0200, Georges Racinet wrote:
> > +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

Oh, there is. I was looking for cpython::RawPyObject and failed.

I'll send V2 shortly.
Georges Racinet - Oct. 13, 2019, 1:25 p.m.
On 10/13/19 2:37 PM, Yuya Nishihara wrote:
> On Sun, 13 Oct 2019 14:08:24 +0200, Georges Racinet wrote:
>>> +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
> Oh, there is. I was looking for cpython::RawPyObject and failed.
That's actually how my first attempt looked like IIRC, but Mark
preferred it to be enclosed in the capsule submodule to limit that
abstraction leak to cases where it's really needed.
>
> I'll send V2 shortly.

Cool, once it's landed, I'll send the remainders of my own series:
removal of `py_set()` in favour of `cpython::objects::PySet`.

Regards,

Patch

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,
+            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,