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