Patchwork [1,of,7,RESEND] rust-cpython: remove useless wrappers from PyLeaked, just move by map()

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 28, 2020, 11:50 p.m.
Message ID <7d298ba617325c96dd00.1580255427@mimosa>
Download mbox | patch
Permalink /patch/44712/
State Accepted
Headers show

Comments

Yuya Nishihara - Jan. 28, 2020, 11:50 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1571711923 -32400
#      Tue Oct 22 11:38:43 2019 +0900
# Node ID 7d298ba617325c96dd00041ecee6d5ebe1b27cc7
# Parent  796d05f3fa84741e42fa00cae53fc9ad5ffc613d
rust-cpython: remove useless wrappers from PyLeaked, just move by map()

This series prepares for migrating to the upstreamed version of PySharedRef.
I found this last batch wasn't queued while rewriting the callers.

While Option<T> was historically needed, it shouldn't be required anymore.
I wasn't aware that each filed can be just moved.
Raphaël Gomès - Jan. 29, 2020, 9:56 a.m.
I had already reviewed these patches last time, so +1 for me, especially 
now that the whole ref-sharing stuff has been upstreamed in 0.4 thanks 
to Yuya.

On 1/29/20 12:50 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1571711923 -32400
> #      Tue Oct 22 11:38:43 2019 +0900
> # Node ID 7d298ba617325c96dd00041ecee6d5ebe1b27cc7
> # Parent  796d05f3fa84741e42fa00cae53fc9ad5ffc613d
> rust-cpython: remove useless wrappers from PyLeaked, just move by map()
>
> This series prepares for migrating to the upstreamed version of PySharedRef.
> I found this last batch wasn't queued while rewriting the callers.
>
> While Option<T> was historically needed, it shouldn't be required anymore.
> I wasn't aware that each filed can be just moved.
>
> 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
> @@ -283,7 +283,7 @@ macro_rules! py_shared_ref {
>   /// borrowed.
>   pub struct PyLeaked<T> {
>       inner: PyObject,
> -    data: Option<T>,
> +    data: T,
>       py_shared_state: &'static PySharedState,
>       /// Generation counter of data `T` captured when PyLeaked is created.
>       generation: usize,
> @@ -305,7 +305,7 @@ impl<T> PyLeaked<T> {
>       ) -> Self {
>           Self {
>               inner: inner.clone_ref(py),
> -            data: Some(data),
> +            data: data,
>               py_shared_state,
>               generation: py_shared_state.current_generation(py),
>           }
> @@ -321,7 +321,7 @@ impl<T> PyLeaked<T> {
>           self.validate_generation(py)?;
>           Ok(PyLeakedRef {
>               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> -            data: self.data.as_ref().unwrap(),
> +            data: &self.data,
>           })
>       }
>   
> @@ -338,7 +338,7 @@ impl<T> PyLeaked<T> {
>           self.validate_generation(py)?;
>           Ok(PyLeakedRefMut {
>               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> -            data: self.data.as_mut().unwrap(),
> +            data: &mut self.data,
>           })
>       }
>   
> @@ -361,7 +361,7 @@ impl<T> PyLeaked<T> {
>       /// corresponding `PyLeaked` is alive. Do not copy it out of the
>       /// function call.
>       pub unsafe fn map<U>(
> -        mut self,
> +        self,
>           py: Python,
>           f: impl FnOnce(T) -> U,
>       ) -> PyLeaked<U> {
> @@ -374,10 +374,10 @@ impl<T> PyLeaked<T> {
>           // In order to make this function safe, maybe we'll need a way to
>           // temporarily restrict the lifetime of self.data and translate the
>           // returned object back to Something<'static>.
> -        let new_data = f(self.data.take().unwrap());
> +        let new_data = f(self.data);
>           PyLeaked {
> -            inner: self.inner.clone_ref(py),
> -            data: Some(new_data),
> +            inner: self.inner,
> +            data: new_data,
>               py_shared_state: self.py_shared_state,
>               generation: self.generation,
>           }
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Jan. 31, 2020, 7:21 p.m.
On Wed, Jan 29, 2020 at 10:56:51AM +0100, Raphaël Gomès wrote:
> I had already reviewed these patches last time, so +1 for me, especially now
> that the whole ref-sharing stuff has been upstreamed in 0.4 thanks to Yuya.

These didn't apply for me, but I just took the patches from today for
rust-cpython stuff. Are these still relevant?

>
> On 1/29/20 12:50 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1571711923 -32400
> > #      Tue Oct 22 11:38:43 2019 +0900
> > # Node ID 7d298ba617325c96dd00041ecee6d5ebe1b27cc7
> > # Parent  796d05f3fa84741e42fa00cae53fc9ad5ffc613d
> > rust-cpython: remove useless wrappers from PyLeaked, just move by map()
> >
> > This series prepares for migrating to the upstreamed version of PySharedRef.
> > I found this last batch wasn't queued while rewriting the callers.
> >
> > While Option<T> was historically needed, it shouldn't be required anymore.
> > I wasn't aware that each filed can be just moved.
> >
> > 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
> > @@ -283,7 +283,7 @@ macro_rules! py_shared_ref {
> >   /// borrowed.
> >   pub struct PyLeaked<T> {
> >       inner: PyObject,
> > -    data: Option<T>,
> > +    data: T,
> >       py_shared_state: &'static PySharedState,
> >       /// Generation counter of data `T` captured when PyLeaked is created.
> >       generation: usize,
> > @@ -305,7 +305,7 @@ impl<T> PyLeaked<T> {
> >       ) -> Self {
> >           Self {
> >               inner: inner.clone_ref(py),
> > -            data: Some(data),
> > +            data: data,
> >               py_shared_state,
> >               generation: py_shared_state.current_generation(py),
> >           }
> > @@ -321,7 +321,7 @@ impl<T> PyLeaked<T> {
> >           self.validate_generation(py)?;
> >           Ok(PyLeakedRef {
> >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > -            data: self.data.as_ref().unwrap(),
> > +            data: &self.data,
> >           })
> >       }
> > @@ -338,7 +338,7 @@ impl<T> PyLeaked<T> {
> >           self.validate_generation(py)?;
> >           Ok(PyLeakedRefMut {
> >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > -            data: self.data.as_mut().unwrap(),
> > +            data: &mut self.data,
> >           })
> >       }
> > @@ -361,7 +361,7 @@ impl<T> PyLeaked<T> {
> >       /// corresponding `PyLeaked` is alive. Do not copy it out of the
> >       /// function call.
> >       pub unsafe fn map<U>(
> > -        mut self,
> > +        self,
> >           py: Python,
> >           f: impl FnOnce(T) -> U,
> >       ) -> PyLeaked<U> {
> > @@ -374,10 +374,10 @@ impl<T> PyLeaked<T> {
> >           // In order to make this function safe, maybe we'll need a way to
> >           // temporarily restrict the lifetime of self.data and translate the
> >           // returned object back to Something<'static>.
> > -        let new_data = f(self.data.take().unwrap());
> > +        let new_data = f(self.data);
> >           PyLeaked {
> > -            inner: self.inner.clone_ref(py),
> > -            data: Some(new_data),
> > +            inner: self.inner,
> > +            data: new_data,
> >               py_shared_state: self.py_shared_state,
> >               generation: self.generation,
> >           }
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - Jan. 31, 2020, 7:51 p.m.
I think I've already queued this series. Sorry that I forgot to say so.

On Fri, Jan 31, 2020 at 11:27 AM Augie Fackler <raf@durin42.com> wrote:

> On Wed, Jan 29, 2020 at 10:56:51AM +0100, Raphaël Gomès wrote:
> > I had already reviewed these patches last time, so +1 for me, especially
> now
> > that the whole ref-sharing stuff has been upstreamed in 0.4 thanks to
> Yuya.
>
> These didn't apply for me, but I just took the patches from today for
> rust-cpython stuff. Are these still relevant?
>
> >
> > On 1/29/20 12:50 AM, Yuya Nishihara wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1571711923 -32400
> > > #      Tue Oct 22 11:38:43 2019 +0900
> > > # Node ID 7d298ba617325c96dd00041ecee6d5ebe1b27cc7
> > > # Parent  796d05f3fa84741e42fa00cae53fc9ad5ffc613d
> > > rust-cpython: remove useless wrappers from PyLeaked, just move by map()
> > >
> > > This series prepares for migrating to the upstreamed version of
> PySharedRef.
> > > I found this last batch wasn't queued while rewriting the callers.
> > >
> > > While Option<T> was historically needed, it shouldn't be required
> anymore.
> > > I wasn't aware that each filed can be just moved.
> > >
> > > 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
> > > @@ -283,7 +283,7 @@ macro_rules! py_shared_ref {
> > >   /// borrowed.
> > >   pub struct PyLeaked<T> {
> > >       inner: PyObject,
> > > -    data: Option<T>,
> > > +    data: T,
> > >       py_shared_state: &'static PySharedState,
> > >       /// Generation counter of data `T` captured when PyLeaked is
> created.
> > >       generation: usize,
> > > @@ -305,7 +305,7 @@ impl<T> PyLeaked<T> {
> > >       ) -> Self {
> > >           Self {
> > >               inner: inner.clone_ref(py),
> > > -            data: Some(data),
> > > +            data: data,
> > >               py_shared_state,
> > >               generation: py_shared_state.current_generation(py),
> > >           }
> > > @@ -321,7 +321,7 @@ impl<T> PyLeaked<T> {
> > >           self.validate_generation(py)?;
> > >           Ok(PyLeakedRef {
> > >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > > -            data: self.data.as_ref().unwrap(),
> > > +            data: &self.data,
> > >           })
> > >       }
> > > @@ -338,7 +338,7 @@ impl<T> PyLeaked<T> {
> > >           self.validate_generation(py)?;
> > >           Ok(PyLeakedRefMut {
> > >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > > -            data: self.data.as_mut().unwrap(),
> > > +            data: &mut self.data,
> > >           })
> > >       }
> > > @@ -361,7 +361,7 @@ impl<T> PyLeaked<T> {
> > >       /// corresponding `PyLeaked` is alive. Do not copy it out of the
> > >       /// function call.
> > >       pub unsafe fn map<U>(
> > > -        mut self,
> > > +        self,
> > >           py: Python,
> > >           f: impl FnOnce(T) -> U,
> > >       ) -> PyLeaked<U> {
> > > @@ -374,10 +374,10 @@ impl<T> PyLeaked<T> {
> > >           // In order to make this function safe, maybe we'll need a
> way to
> > >           // temporarily restrict the lifetime of self.data and
> translate the
> > >           // returned object back to Something<'static>.
> > > -        let new_data = f(self.data.take().unwrap());
> > > +        let new_data = f(self.data);
> > >           PyLeaked {
> > > -            inner: self.inner.clone_ref(py),
> > > -            data: Some(new_data),
> > > +            inner: self.inner,
> > > +            data: new_data,
> > >               py_shared_state: self.py_shared_state,
> > >               generation: self.generation,
> > >           }
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel@mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - Jan. 31, 2020, 7:51 p.m.
No worries, thanks!

> On Jan 31, 2020, at 14:51, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> I think I've already queued this series. Sorry that I forgot to say so.
> 
> On Fri, Jan 31, 2020 at 11:27 AM Augie Fackler <raf@durin42.com> wrote:
> On Wed, Jan 29, 2020 at 10:56:51AM +0100, Raphaël Gomès wrote:
> > I had already reviewed these patches last time, so +1 for me, especially now
> > that the whole ref-sharing stuff has been upstreamed in 0.4 thanks to Yuya.
> 
> These didn't apply for me, but I just took the patches from today for
> rust-cpython stuff. Are these still relevant?
> 
> >
> > On 1/29/20 12:50 AM, Yuya Nishihara wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1571711923 -32400
> > > #      Tue Oct 22 11:38:43 2019 +0900
> > > # Node ID 7d298ba617325c96dd00041ecee6d5ebe1b27cc7
> > > # Parent  796d05f3fa84741e42fa00cae53fc9ad5ffc613d
> > > rust-cpython: remove useless wrappers from PyLeaked, just move by map()
> > >
> > > This series prepares for migrating to the upstreamed version of PySharedRef.
> > > I found this last batch wasn't queued while rewriting the callers.
> > >
> > > While Option<T> was historically needed, it shouldn't be required anymore.
> > > I wasn't aware that each filed can be just moved.
> > >
> > > 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
> > > @@ -283,7 +283,7 @@ macro_rules! py_shared_ref {
> > >   /// borrowed.
> > >   pub struct PyLeaked<T> {
> > >       inner: PyObject,
> > > -    data: Option<T>,
> > > +    data: T,
> > >       py_shared_state: &'static PySharedState,
> > >       /// Generation counter of data `T` captured when PyLeaked is created.
> > >       generation: usize,
> > > @@ -305,7 +305,7 @@ impl<T> PyLeaked<T> {
> > >       ) -> Self {
> > >           Self {
> > >               inner: inner.clone_ref(py),
> > > -            data: Some(data),
> > > +            data: data,
> > >               py_shared_state,
> > >               generation: py_shared_state.current_generation(py),
> > >           }
> > > @@ -321,7 +321,7 @@ impl<T> PyLeaked<T> {
> > >           self.validate_generation(py)?;
> > >           Ok(PyLeakedRef {
> > >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > > -            data: self.data.as_ref().unwrap(),
> > > +            data: &self.data,
> > >           })
> > >       }
> > > @@ -338,7 +338,7 @@ impl<T> PyLeaked<T> {
> > >           self.validate_generation(py)?;
> > >           Ok(PyLeakedRefMut {
> > >               _borrow: BorrowPyShared::new(py, self.py_shared_state),
> > > -            data: self.data.as_mut().unwrap(),
> > > +            data: &mut self.data,
> > >           })
> > >       }
> > > @@ -361,7 +361,7 @@ impl<T> PyLeaked<T> {
> > >       /// corresponding `PyLeaked` is alive. Do not copy it out of the
> > >       /// function call.
> > >       pub unsafe fn map<U>(
> > > -        mut self,
> > > +        self,
> > >           py: Python,
> > >           f: impl FnOnce(T) -> U,
> > >       ) -> PyLeaked<U> {
> > > @@ -374,10 +374,10 @@ impl<T> PyLeaked<T> {
> > >           // In order to make this function safe, maybe we'll need a way to
> > >           // temporarily restrict the lifetime of self.data and translate the
> > >           // returned object back to Something<'static>.
> > > -        let new_data = f(self.data.take().unwrap());
> > > +        let new_data = f(self.data);
> > >           PyLeaked {
> > > -            inner: self.inner.clone_ref(py),
> > > -            data: Some(new_data),
> > > +            inner: self.inner,
> > > +            data: new_data,
> > >               py_shared_state: self.py_shared_state,
> > >               generation: self.generation,
> > >           }
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel@mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

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
@@ -283,7 +283,7 @@  macro_rules! py_shared_ref {
 /// borrowed.
 pub struct PyLeaked<T> {
     inner: PyObject,
-    data: Option<T>,
+    data: T,
     py_shared_state: &'static PySharedState,
     /// Generation counter of data `T` captured when PyLeaked is created.
     generation: usize,
@@ -305,7 +305,7 @@  impl<T> PyLeaked<T> {
     ) -> Self {
         Self {
             inner: inner.clone_ref(py),
-            data: Some(data),
+            data: data,
             py_shared_state,
             generation: py_shared_state.current_generation(py),
         }
@@ -321,7 +321,7 @@  impl<T> PyLeaked<T> {
         self.validate_generation(py)?;
         Ok(PyLeakedRef {
             _borrow: BorrowPyShared::new(py, self.py_shared_state),
-            data: self.data.as_ref().unwrap(),
+            data: &self.data,
         })
     }
 
@@ -338,7 +338,7 @@  impl<T> PyLeaked<T> {
         self.validate_generation(py)?;
         Ok(PyLeakedRefMut {
             _borrow: BorrowPyShared::new(py, self.py_shared_state),
-            data: self.data.as_mut().unwrap(),
+            data: &mut self.data,
         })
     }
 
@@ -361,7 +361,7 @@  impl<T> PyLeaked<T> {
     /// corresponding `PyLeaked` is alive. Do not copy it out of the
     /// function call.
     pub unsafe fn map<U>(
-        mut self,
+        self,
         py: Python,
         f: impl FnOnce(T) -> U,
     ) -> PyLeaked<U> {
@@ -374,10 +374,10 @@  impl<T> PyLeaked<T> {
         // In order to make this function safe, maybe we'll need a way to
         // temporarily restrict the lifetime of self.data and translate the
         // returned object back to Something<'static>.
-        let new_data = f(self.data.take().unwrap());
+        let new_data = f(self.data);
         PyLeaked {
-            inner: self.inner.clone_ref(py),
-            data: Some(new_data),
+            inner: self.inner,
+            data: new_data,
             py_shared_state: self.py_shared_state,
             generation: self.generation,
         }