Patchwork D5441: rust-cpython: binding for LazyAncestors

login
register
mail settings
Submitter phabricator
Date Dec. 22, 2018, 4:24 p.m.
Message ID <ce176da276bcb1ed1c5e609febfba74f@localhost.localdomain>
Download mbox | patch
Permalink /patch/37315/
State Not Applicable
Headers show

Comments

phabricator - Dec. 22, 2018, 4:24 p.m.
gracinet updated this revision to Diff 12952.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5441?vs=12878&id=12952

REVISION DETAIL
  https://phab.mercurial-scm.org/D5441

AFFECTED FILES
  rust/hg-cpython/src/ancestors.rs
  tests/test-rust-ancestor.py

CHANGE DETAILS




To: gracinet, #hg-reviewers, kevincox
Cc: yuja, durin42, kevincox, mercurial-devel
Yuya Nishihara - Dec. 24, 2018, 4:56 a.m.
> +/// The purpose of this module is to hide identifiers from other Rust users
> +///
> +/// Some of the identifiers we are defining are meant for consumption
> +/// from Python with other naming conventions. For instance, `py_class!`
> +/// does not let us make a distinction between the Python and the Rust name,
> +/// and that name ends up in particular in the `__class__` attribute, so that
> +/// renaming it inside its Python module is not enough.
> +///
> +/// For Rust consumers, we will reexport these definitions, following the
> +/// appropriate naming convention.
> +mod concealed_detail {
> +    use super::*;
> +
> +    py_class!(pub class lazyancestors |py| {

Does the class name actually matter? Personally I don't care if
`lazyancestors()` function returns a `LazyAncestors` object. We'll anyway
need a wrapper function to make pure ancestors and rustext ancestors
compatible.
phabricator - Dec. 24, 2018, 4:57 a.m.
yuja added a comment.


  > +/// The purpose of this module is to hide identifiers from other Rust users
  >  +///
  >  +/// Some of the identifiers we are defining are meant for consumption
  >  +/// from Python with other naming conventions. For instance, `py_class!`
  >  +/// does not let us make a distinction between the Python and the Rust name,
  >  +/// and that name ends up in particular in the `__class__` attribute, so that
  >  +/// renaming it inside its Python module is not enough.
  >  +///
  >  +/// For Rust consumers, we will reexport these definitions, following the
  >  +/// appropriate naming convention.
  >  +mod concealed_detail {
  >  +    use super::*;
  >  +
  >  +    py_class!(pub class lazyancestors |py| {
  
  Does the class name actually matter? Personally I don't care if
  `lazyancestors()` function returns a `LazyAncestors` object. We'll anyway
  need a wrapper function to make pure ancestors and rustext ancestors
  compatible.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5441

To: gracinet, #hg-reviewers, kevincox
Cc: yuja, durin42, kevincox, mercurial-devel

Patch

diff --git a/tests/test-rust-ancestor.py b/tests/test-rust-ancestor.py
--- a/tests/test-rust-ancestor.py
+++ b/tests/test-rust-ancestor.py
@@ -9,7 +9,10 @@ 
     rustext = None
 else:
     # this would fail already without appropriate ancestor.__package__
-    from mercurial.rustext.ancestor import AncestorsIterator
+    from mercurial.rustext.ancestor import (
+        AncestorsIterator,
+        lazyancestors
+    )
 
 try:
     from mercurial.cext import parsers as cparsers
@@ -71,6 +74,37 @@ 
         ait = AncestorsIterator(idx, [3], 0, False)
         self.assertEqual([r for r in ait], [2, 1, 0])
 
+    def testlazyancestors(self):
+        idx = self.parseindex()
+        start_count = sys.getrefcount(idx)  # should be 2 (see Python doc)
+        self.assertEqual({i: (r[5], r[6]) for i, r in enumerate(idx)},
+                         {0: (-1, -1),
+                          1: (0, -1),
+                          2: (1, -1),
+                          3: (2, -1)})
+        lazy = lazyancestors(idx, [3], 0, True)
+        # we have two more references to the index:
+        # - in its inner iterator for __contains__ and __bool__
+        # - in the lazyancestors instance itself (to spawn new iterators)
+        self.assertEqual(sys.getrefcount(idx), start_count + 2)
+
+        self.assertTrue(2 in lazy)
+        self.assertTrue(bool(lazy))
+        self.assertEqual([r for r in lazy], [3, 2, 1, 0])
+        # a second time to validate that we spawn new iterators
+        self.assertEqual([r for r in lazy], [3, 2, 1, 0])
+
+        # now let's watch the refcounts closer
+        ait = iter(lazy)
+        self.assertEqual(sys.getrefcount(idx), start_count + 3)
+        del ait
+        self.assertEqual(sys.getrefcount(idx), start_count + 2)
+        del lazy
+        self.assertEqual(sys.getrefcount(idx), start_count)
+
+        # let's check bool for an empty one
+        self.assertFalse(lazyancestors(idx, [0], 0, False))
+
     def testrefcount(self):
         idx = self.parseindex()
         start_count = sys.getrefcount(idx)
diff --git a/rust/hg-cpython/src/ancestors.rs b/rust/hg-cpython/src/ancestors.rs
--- a/rust/hg-cpython/src/ancestors.rs
+++ b/rust/hg-cpython/src/ancestors.rs
@@ -12,8 +12,8 @@ 
 };
 use exceptions::GraphError;
 use hg;
-use hg::AncestorsIterator as CoreIterator;
 use hg::Revision;
+use hg::{AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy};
 use std::cell::RefCell;
 
 /// Utility function to convert a Python iterable into a Vec<Revision>
@@ -27,7 +27,7 @@ 
         .collect()
 }
 
-py_class!(class AncestorsIterator |py| {
+py_class!(pub class AncestorsIterator |py| {
     // TODO RW lock ?
     data inner: RefCell<Box<CoreIterator<Index>>>;
 
@@ -69,6 +69,52 @@ 
     }
 }
 
+/// The purpose of this module is to hide identifiers from other Rust users
+///
+/// Some of the identifiers we are defining are meant for consumption
+/// from Python with other naming conventions. For instance, `py_class!`
+/// does not let us make a distinction between the Python and the Rust name,
+/// and that name ends up in particular in the `__class__` attribute, so that
+/// renaming it inside its Python module is not enough.
+///
+/// For Rust consumers, we will reexport these definitions, following the
+/// appropriate naming convention.
+mod concealed_detail {
+    use super::*;
+
+    py_class!(pub class lazyancestors |py| {
+        data inner: RefCell<Box<CoreLazy<Index>>>;
+
+        def __contains__(&self, rev: Revision) -> PyResult<bool> {
+            self.inner(py).borrow_mut().contains(rev).map_err(|e| GraphError::pynew(py, e))
+        }
+
+        def __iter__(&self) -> PyResult<AncestorsIterator> {
+            AncestorsIterator::from_inner(py,
+                                          self.inner(py).borrow().iter())
+        }
+
+        def __bool__(&self) -> PyResult<bool> {
+            Ok(!self.inner(py).borrow().is_empty())
+        }
+
+        def __new__(_cls, index: PyObject, initrevs: PyObject,
+                    stoprev: Revision, inclusive: bool) -> PyResult<Self> {
+            let initvec = reviter_to_revvec(py, initrevs)?;
+            let lazy = match CoreLazy::new(
+                Index::new(py, index)?, initvec, stoprev, inclusive) {
+                Ok(lazy) => lazy,
+                Err(e) => {
+                    return Err(GraphError::new(py, format!("{:?}", e)));
+                }
+            };
+            Self::create_instance(py, RefCell::new(Box::new(lazy)))
+        }
+    });
+}
+
+pub use self::concealed_detail::lazyancestors as LazyAncestors;
+
 /// Create the module, with __package__ given from parent
 pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
     let dotted_name = &format!("{}.ancestor", package);
@@ -80,6 +126,7 @@ 
         "Generic DAG ancestor algorithms - Rust implementation",
     )?;
     m.add_class::<AncestorsIterator>(py)?;
+    m.add_class::<LazyAncestors>(py)?;
 
     let sys = PyModule::import(py, "sys")?;
     let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;