D7120: rust-cpython: removed now useless py_set() conversion

gracinet (Georges Racinet) phabricator at mercurial-scm.org
Wed Oct 16 11:26:21 EDT 2019


gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  In rust-cpython 0.3.0, HashSets implement the appropriate
  ToPythonObject, we can therefore get rid of this hacky conversion.
  
  There still remains an inefficiency in `MissingAncestors.bases()`:
  we have to clone, because `to_py_object()` requires full ownership.
  However:
  
  - the only use case outside of unit tests used to be from
  
  `setdiscovery.partialdiscovery` which is now fully implemented
  in Rust.
  
  - it's not worse than what `py_set()` used to do

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  rust/hg-cpython/src/ancestors.rs
  rust/hg-cpython/src/conversion.rs
  rust/hg-cpython/src/dagops.rs
  rust/hg-cpython/src/discovery.rs

CHANGE DETAILS

diff --git a/rust/hg-cpython/src/discovery.rs b/rust/hg-cpython/src/discovery.rs
--- a/rust/hg-cpython/src/discovery.rs
+++ b/rust/hg-cpython/src/discovery.rs
@@ -13,9 +13,7 @@
 //!   `mercurial.setdiscovery.partialdiscovery`.
 
 use crate::{
-    cindex::Index,
-    conversion::{py_set, rev_pyiter_collect},
-    exceptions::GraphError,
+    cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
 };
 use cpython::{
     ObjectProtocol, PyDict, PyModule, PyObject, PyResult, PyTuple, Python,
@@ -23,6 +21,7 @@
 };
 use hg::discovery::PartialDiscovery as CorePartialDiscovery;
 use hg::Revision;
+use std::collections::HashSet;
 
 use std::cell::RefCell;
 
@@ -106,12 +105,9 @@
         Ok(as_dict)
     }
 
-    def commonheads(&self) -> PyResult<PyObject> {
-        py_set(
-            py,
-            &self.inner(py).borrow().common_heads()
-                .map_err(|e| GraphError::pynew(py, e))?
-        )
+    def commonheads(&self) -> PyResult<HashSet<Revision>> {
+        self.inner(py).borrow().common_heads()
+            .map_err(|e| GraphError::pynew(py, e))
     }
 
     def takefullsample(&self, _headrevs: PyObject,
diff --git a/rust/hg-cpython/src/dagops.rs b/rust/hg-cpython/src/dagops.rs
--- a/rust/hg-cpython/src/dagops.rs
+++ b/rust/hg-cpython/src/dagops.rs
@@ -10,9 +10,7 @@
 //!
 //! From Python, this will be seen as `mercurial.rustext.dagop`
 use crate::{
-    cindex::Index,
-    conversion::{py_set, rev_pyiter_collect},
-    exceptions::GraphError,
+    cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
 };
 use cpython::{PyDict, PyModule, PyObject, PyResult, Python};
 use hg::dagops;
@@ -26,11 +24,11 @@
     py: Python,
     index: PyObject,
     revs: PyObject,
-) -> PyResult<PyObject> {
+) -> PyResult<HashSet<Revision>> {
     let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs)?;
     dagops::retain_heads(&Index::new(py, index)?, &mut as_set)
         .map_err(|e| GraphError::pynew(py, e))?;
-    py_set(py, &as_set)
+    Ok(as_set)
 }
 
 /// Create the module, with `__package__` given from parent
diff --git a/rust/hg-cpython/src/conversion.rs b/rust/hg-cpython/src/conversion.rs
--- a/rust/hg-cpython/src/conversion.rs
+++ b/rust/hg-cpython/src/conversion.rs
@@ -8,12 +8,8 @@
 //! Bindings for the hg::ancestors module provided by the
 //! `hg-core` crate. From Python, this will be seen as `rustext.ancestor`
 
-use cpython::{
-    ObjectProtocol, PyDict, PyObject, PyResult, PyTuple, Python, PythonObject,
-    ToPyObject,
-};
+use cpython::{ObjectProtocol, PyObject, PyResult, Python};
 use hg::Revision;
-use std::collections::HashSet;
 use std::iter::FromIterator;
 
 /// Utility function to convert a Python iterable into various collections
@@ -30,21 +26,3 @@
         .map(|r| r.and_then(|o| o.extract::<Revision>(py)))
         .collect()
 }
-
-/// Copy and convert an `HashSet<Revision>` in a Python set
-///
-/// This will probably turn useless once `PySet` support lands in
-/// `rust-cpython`.
-///
-/// This builds a Python tuple, then calls Python's "set()" on it
-pub fn py_set(py: Python, set: &HashSet<Revision>) -> PyResult<PyObject> {
-    let as_vec: Vec<PyObject> = set
-        .iter()
-        .map(|rev| rev.to_py_object(py).into_object())
-        .collect();
-    let as_pytuple = PyTuple::new(py, as_vec.as_slice());
-
-    let locals = PyDict::new(py);
-    locals.set_item(py, "obj", as_pytuple.to_py_object(py))?;
-    py.eval("set(obj)", None, Some(&locals))
-}
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
@@ -35,9 +35,7 @@
 //! [`MissingAncestors`]: struct.MissingAncestors.html
 //! [`AncestorsIterator`]: struct.AncestorsIterator.html
 use crate::{
-    cindex::Index,
-    conversion::{py_set, rev_pyiter_collect},
-    exceptions::GraphError,
+    cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
 };
 use cpython::{
     ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject, PyResult,
@@ -146,13 +144,13 @@
         Ok(py.None())
     }
 
-    def bases(&self) -> PyResult<PyObject> {
-        py_set(py, self.inner(py).borrow().get_bases())
+    def bases(&self) -> PyResult<HashSet<Revision>> {
+        Ok(self.inner(py).borrow().get_bases().clone())
     }
 
-    def basesheads(&self) -> PyResult<PyObject> {
+    def basesheads(&self) -> PyResult<HashSet<Revision>> {
         let inner = self.inner(py).borrow();
-        py_set(py, &inner.bases_heads().map_err(|e| GraphError::pynew(py, e))?)
+        inner.bases_heads().map_err(|e| GraphError::pynew(py, e))
     }
 
     def removeancestorsfrom(&self, revs: PyObject) -> PyResult<PyObject> {



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


More information about the Mercurial-devel mailing list