Patchwork [05,of,10] rust-cpython: move $leaked struct out of macro

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 22, 2019, 6:41 a.m.
Message ID <b941b6d280e6f66e6536.1569134502@mimosa>
Download mbox | patch
Permalink /patch/41724/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 22, 2019, 6:41 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1567942015 -32400
#      Sun Sep 08 20:26:55 2019 +0900
# Node ID b941b6d280e6f66e6536df80864eda1e9dfbffce
# Parent  cec8317e90342e922cfc8228aa268135ad13d26f
rust-cpython: move $leaked struct out of macro

It wasn't easy to hack the $leaked struct since errors in macro would
generate lots of compile errors. Let's make it a plain struct so we can
easily extend it.

PyLeakedRef keeps a more generic PyObject instead of the $name struct
since it no longer has to call any specific methods implemented by
the $name class. $leaked parameter in py_shared_iterator!() is kept
for future change.

Patch

diff --git a/rust/hg-cpython/src/dirstate/copymap.rs b/rust/hg-cpython/src/dirstate/copymap.rs
--- a/rust/hg-cpython/src/dirstate/copymap.rs
+++ b/rust/hg-cpython/src/dirstate/copymap.rs
@@ -11,7 +11,8 @@ 
 use cpython::{PyBytes, PyClone, PyDict, PyObject, PyResult, Python};
 use std::cell::RefCell;
 
-use crate::dirstate::dirstate_map::{DirstateMap, DirstateMapLeakedRef};
+use crate::dirstate::dirstate_map::DirstateMap;
+use crate::ref_sharing::PyLeakedRef;
 use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
 
 py_class!(pub class CopyMap |py| {
@@ -103,7 +104,7 @@  impl CopyMap {
 
 py_shared_iterator!(
     CopyMapKeysIterator,
-    DirstateMapLeakedRef,
+    PyLeakedRef,
     CopyMapIter<'static>,
     CopyMap::translate_key,
     Option<PyBytes>
@@ -111,7 +112,7 @@  py_shared_iterator!(
 
 py_shared_iterator!(
     CopyMapItemsIterator,
-    DirstateMapLeakedRef,
+    PyLeakedRef,
     CopyMapIter<'static>,
     CopyMap::translate_key_value,
     Option<(PyBytes, PyBytes)>
diff --git a/rust/hg-cpython/src/dirstate/dirs_multiset.rs b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
@@ -16,7 +16,8 @@  use cpython::{
     Python,
 };
 
-use crate::{dirstate::extract_dirstate, ref_sharing::PySharedRefCell};
+use crate::dirstate::extract_dirstate;
+use crate::ref_sharing::{PyLeakedRef, PySharedRefCell};
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirsMultisetIter, DirstateMapError, DirstateParseError,
@@ -106,7 +107,7 @@  py_class!(pub class Dirs |py| {
     }
 });
 
-py_shared_ref!(Dirs, DirsMultiset, inner, DirsMultisetLeakedRef,);
+py_shared_ref!(Dirs, DirsMultiset, inner);
 
 impl Dirs {
     pub fn from_inner(py: Python, d: DirsMultiset) -> PyResult<Self> {
@@ -123,7 +124,7 @@  impl Dirs {
 
 py_shared_iterator!(
     DirsMultisetKeysIterator,
-    DirsMultisetLeakedRef,
+    PyLeakedRef,
     DirsMultisetIter<'static>,
     Dirs::translate_key,
     Option<PyBytes>
diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -21,7 +21,7 @@  use libc::c_char;
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
     dirstate::{decapsule_make_dirstate_tuple, dirs_multiset::Dirs},
-    ref_sharing::PySharedRefCell,
+    ref_sharing::{PyLeakedRef, PySharedRefCell},
 };
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
@@ -501,11 +501,11 @@  impl DirstateMap {
     }
 }
 
-py_shared_ref!(DirstateMap, RustDirstateMap, inner, DirstateMapLeakedRef,);
+py_shared_ref!(DirstateMap, RustDirstateMap, inner);
 
 py_shared_iterator!(
     DirstateMapKeysIterator,
-    DirstateMapLeakedRef,
+    PyLeakedRef,
     StateMapIter<'static>,
     DirstateMap::translate_key,
     Option<PyBytes>
@@ -513,7 +513,7 @@  py_shared_iterator!(
 
 py_shared_iterator!(
     DirstateMapItemsIterator,
-    DirstateMapLeakedRef,
+    PyLeakedRef,
     StateMapIter<'static>,
     DirstateMap::translate_key_value,
     Option<(PyBytes, PyObject)>
diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -8,7 +8,7 @@ 
 //! Macros for use in the `hg-cpython` bridge library.
 
 use crate::exceptions::AlreadyBorrowed;
-use cpython::{PyResult, Python};
+use cpython::{PyClone, PyObject, PyResult, Python};
 use std::cell::{Cell, Ref, RefCell, RefMut};
 
 /// Manages the shared state between Python and Rust
@@ -204,9 +204,6 @@  impl<'a, T> Drop for PyRefMut<'a, T> {
 /// * `$inner_struct` is the identifier of the underlying Rust struct
 /// * `$data_member` is the identifier of the data member of `$inner_struct`
 /// that will be shared.
-/// * `$leaked` is the identifier to give to the struct that will manage
-/// references to `$name`, to be used for example in other macros like
-/// `py_shared_iterator`.
 ///
 /// # Example
 ///
@@ -219,14 +216,13 @@  impl<'a, T> Drop for PyRefMut<'a, T> {
 ///     data inner: PySharedRefCell<MyStruct>;
 /// });
 ///
-/// py_shared_ref!(MyType, MyStruct, inner, MyTypeLeakedRef);
+/// py_shared_ref!(MyType, MyStruct, inner);
 /// ```
 macro_rules! py_shared_ref {
     (
         $name: ident,
         $inner_struct: ident,
-        $data_member: ident,
-        $leaked: ident,
+        $data_member: ident
     ) => {
         impl $name {
             // TODO: remove this function in favor of inner(py).borrow_mut()
@@ -251,59 +247,59 @@  macro_rules! py_shared_ref {
             unsafe fn leak_immutable<'a>(
                 &'a self,
                 py: Python<'a>,
-            ) -> PyResult<($leaked, &'static $inner_struct)> {
+            ) -> PyResult<(PyLeakedRef, &'static $inner_struct)> {
+                use cpython::PythonObject;
                 // assert $data_member type
                 use crate::ref_sharing::PySharedRefCell;
                 let data: &PySharedRefCell<_> = self.$data_member(py);
                 let (static_ref, static_state_ref) =
                     data.py_shared_state.leak_immutable(py, data)?;
-                let leak_handle = $leaked::new(py, self, static_state_ref);
+                let leak_handle =
+                    PyLeakedRef::new(py, self.as_object(), static_state_ref);
                 Ok((leak_handle, static_ref))
             }
         }
+    };
+}
 
-        /// Manage immutable references to `$name` leaked into Python
-        /// iterators.
-        ///
-        /// In truth, this does not represent leaked references themselves;
-        /// it is instead useful alongside them to manage them.
-        pub struct $leaked {
-            _inner: $name,
-            py_shared_state: &'static crate::ref_sharing::PySharedState,
-        }
+/// Manage immutable references to `PyObject` leaked into Python iterators.
+///
+/// In truth, this does not represent leaked references themselves;
+/// it is instead useful alongside them to manage them.
+pub struct PyLeakedRef {
+    _inner: PyObject,
+    py_shared_state: &'static PySharedState,
+}
 
-        impl $leaked {
-            /// # Safety
-            ///
-            /// The `py_shared_state` must be owned by the `inner` Python
-            /// object.
-            // Marked as unsafe so client code wouldn't construct $leaked
-            // struct by mistake. Its drop() is unsafe.
-            unsafe fn new(
-                py: Python,
-                inner: &$name,
-                py_shared_state: &'static crate::ref_sharing::PySharedState,
-            ) -> Self {
-                Self {
-                    _inner: inner.clone_ref(py),
-                    py_shared_state,
-                }
-            }
+impl PyLeakedRef {
+    /// # Safety
+    ///
+    /// The `py_shared_state` must be owned by the `inner` Python object.
+    // Marked as unsafe so client code wouldn't construct PyLeakedRef
+    // struct by mistake. Its drop() is unsafe.
+    pub unsafe fn new(
+        py: Python,
+        inner: &PyObject,
+        py_shared_state: &'static PySharedState,
+    ) -> Self {
+        Self {
+            _inner: inner.clone_ref(py),
+            py_shared_state,
         }
+    }
+}
 
-        impl Drop for $leaked {
-            fn drop(&mut self) {
-                // py_shared_state should be alive since we do have
-                // a Python reference to the owner object. Taking GIL makes
-                // sure that the state is only accessed by this thread.
-                let gil = Python::acquire_gil();
-                let py = gil.python();
-                unsafe {
-                    self.py_shared_state.decrease_leak_count(py, false);
-                }
-            }
+impl Drop for PyLeakedRef {
+    fn drop(&mut self) {
+        // py_shared_state should be alive since we do have
+        // a Python reference to the owner object. Taking GIL makes
+        // sure that the state is only accessed by this thread.
+        let gil = Python::acquire_gil();
+        let py = gil.python();
+        unsafe {
+            self.py_shared_state.decrease_leak_count(py, false);
         }
-    };
+    }
 }
 
 /// Defines a `py_class!` that acts as a Python iterator over a Rust iterator.
@@ -357,7 +353,7 @@  macro_rules! py_shared_ref {
 ///
 /// py_shared_iterator!(
 ///     MyTypeItemsIterator,
-///     MyTypeLeakedRef,
+///     PyLeakedRef,
 ///     HashMap<'static, Vec<u8>, Vec<u8>>,
 ///     MyType::translate_key_value,
 ///     Option<(PyBytes, PyBytes)>
@@ -366,7 +362,7 @@  macro_rules! py_shared_ref {
 macro_rules! py_shared_iterator {
     (
         $name: ident,
-        $leaked: ident,
+        $leaked: ty,
         $iterator_type: ty,
         $success_func: expr,
         $success_type: ty