This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7120

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

Authored by gracinet on Oct 16 2019, 11:25 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG33fe96a5c522: rust-cpython: removed now useless py_set() conversion
rHG4fc02c926e88: rust-cpython: removed now useless py_set() conversion
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

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Oct 16 2019, 11:25 AM
gracinet added a commit: rHG4fc02c926e88: rust-cpython: removed now useless py_set() conversion.Oct 16 2019, 5:21 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG4fc02c926e88: rust-cpython: removed now useless py_set() conversion (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 17265

rust/hg-cpython/src/ancestors.rs

//! `ancestor._lazyancestorsiter` Python generator. From Python, instances of //! `ancestor._lazyancestorsiter` Python generator. From Python, instances of
//! this should be mainly obtained by calling `iter()` on a [`LazyAncestors`] //! this should be mainly obtained by calling `iter()` on a [`LazyAncestors`]
//! instance. //! instance.
//! //!
//! [`LazyAncestors`]: struct.LazyAncestors.html //! [`LazyAncestors`]: struct.LazyAncestors.html
//! [`MissingAncestors`]: struct.MissingAncestors.html //! [`MissingAncestors`]: struct.MissingAncestors.html
//! [`AncestorsIterator`]: struct.AncestorsIterator.html //! [`AncestorsIterator`]: struct.AncestorsIterator.html
use crate::{ use crate::{
cindex::Index, cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
conversion::{py_set, rev_pyiter_collect},
exceptions::GraphError,
}; };
use cpython::{ use cpython::{
ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject, PyResult, ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject, PyResult,
Python, PythonObject, ToPyObject, Python, PythonObject, ToPyObject,
}; };
use hg::Revision; use hg::Revision;
use hg::{ use hg::{
AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy, AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy,
let bases_vec: Vec<Revision> = rev_pyiter_collect(py, &bases)?; let bases_vec: Vec<Revision> = rev_pyiter_collect(py, &bases)?;
inner.add_bases(bases_vec); inner.add_bases(bases_vec);
// cpython doc has examples with PyResult<()> but this gives me // cpython doc has examples with PyResult<()> but this gives me
// the trait `cpython::ToPyObject` is not implemented for `()` // the trait `cpython::ToPyObject` is not implemented for `()`
// so let's return an explicit None // so let's return an explicit None
Ok(py.None()) Ok(py.None())
} }
def bases(&self) -> PyResult<PyObject> { def bases(&self) -> PyResult<HashSet<Revision>> {
py_set(py, self.inner(py).borrow().get_bases()) 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(); 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> { def removeancestorsfrom(&self, revs: PyObject) -> PyResult<PyObject> {
let mut inner = self.inner(py).borrow_mut(); let mut inner = self.inner(py).borrow_mut();
// this is very lame: we convert to a Rust set, update it in place // this is very lame: we convert to a Rust set, update it in place
// and then convert back to Python, only to have Python remove the // and then convert back to Python, only to have Python remove the
// excess (thankfully, Python is happy with a list or even an iterator) // excess (thankfully, Python is happy with a list or even an iterator)
// Leads to improve this: // Leads to improve this:

rust/hg-cpython/src/conversion.rs

// conversion.rs // conversion.rs
// //
// Copyright 2019 Georges Racinet <georges.racinet@octobus.net> // Copyright 2019 Georges Racinet <georges.racinet@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings for the hg::ancestors module provided by the //! Bindings for the hg::ancestors module provided by the
//! `hg-core` crate. From Python, this will be seen as `rustext.ancestor` //! `hg-core` crate. From Python, this will be seen as `rustext.ancestor`
use cpython::{ use cpython::{ObjectProtocol, PyObject, PyResult, Python};
ObjectProtocol, PyDict, PyObject, PyResult, PyTuple, Python, PythonObject,
ToPyObject,
};
use hg::Revision; use hg::Revision;
use std::collections::HashSet;
use std::iter::FromIterator; use std::iter::FromIterator;
/// Utility function to convert a Python iterable into various collections /// Utility function to convert a Python iterable into various collections
/// ///
/// We need this in particular to feed to various methods of inner objects /// We need this in particular to feed to various methods of inner objects
/// with `impl IntoIterator<Item=Revision>` arguments, because /// with `impl IntoIterator<Item=Revision>` arguments, because
/// a `PyErr` can arise at each step of iteration, whereas these methods /// a `PyErr` can arise at each step of iteration, whereas these methods
/// expect iterables over `Revision`, not over some `Result<Revision, PyErr>` /// expect iterables over `Revision`, not over some `Result<Revision, PyErr>`
pub fn rev_pyiter_collect<C>(py: Python, revs: &PyObject) -> PyResult<C> pub fn rev_pyiter_collect<C>(py: Python, revs: &PyObject) -> PyResult<C>
where where
C: FromIterator<Revision>, C: FromIterator<Revision>,
{ {
revs.iter(py)? revs.iter(py)?
.map(|r| r.and_then(|o| o.extract::<Revision>(py))) .map(|r| r.and_then(|o| o.extract::<Revision>(py)))
.collect() .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))
}

rust/hg-cpython/src/dagops.rs

// dagops.rs // dagops.rs
// //
// Copyright 2019 Georges Racinet <georges.racinet@octobus.net> // Copyright 2019 Georges Racinet <georges.racinet@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings for the `hg::dagops` module provided by the //! Bindings for the `hg::dagops` module provided by the
//! `hg-core` package. //! `hg-core` package.
//! //!
//! From Python, this will be seen as `mercurial.rustext.dagop` //! From Python, this will be seen as `mercurial.rustext.dagop`
use crate::{ use crate::{
cindex::Index, cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
conversion::{py_set, rev_pyiter_collect},
exceptions::GraphError,
}; };
use cpython::{PyDict, PyModule, PyObject, PyResult, Python}; use cpython::{PyDict, PyModule, PyObject, PyResult, Python};
use hg::dagops; use hg::dagops;
use hg::Revision; use hg::Revision;
use std::collections::HashSet; use std::collections::HashSet;
/// Using the the `index`, return heads out of any Python iterable of Revisions /// Using the the `index`, return heads out of any Python iterable of Revisions
/// ///
/// This is the Rust counterpart for `mercurial.dagop.headrevs` /// This is the Rust counterpart for `mercurial.dagop.headrevs`
pub fn headrevs( pub fn headrevs(
py: Python, py: Python,
index: PyObject, index: PyObject,
revs: PyObject, revs: PyObject,
) -> PyResult<PyObject> { ) -> PyResult<HashSet<Revision>> {
let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs)?; let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs)?;
dagops::retain_heads(&Index::new(py, index)?, &mut as_set) dagops::retain_heads(&Index::new(py, index)?, &mut as_set)
.map_err(|e| GraphError::pynew(py, e))?; .map_err(|e| GraphError::pynew(py, e))?;
py_set(py, &as_set) Ok(as_set)
} }
/// Create the module, with `__package__` given from parent /// Create the module, with `__package__` given from parent
pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> { pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
let dotted_name = &format!("{}.dagop", package); let dotted_name = &format!("{}.dagop", package);
let m = PyModule::new(py, dotted_name)?; let m = PyModule::new(py, dotted_name)?;
m.add(py, "__package__", package)?; m.add(py, "__package__", package)?;
m.add(py, "__doc__", "DAG operations - Rust implementation")?; m.add(py, "__doc__", "DAG operations - Rust implementation")?;

rust/hg-cpython/src/discovery.rs

// discovery.rs // discovery.rs
// //
// Copyright 2018 Georges Racinet <gracinet@anybox.fr> // Copyright 2018 Georges Racinet <gracinet@anybox.fr>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings for the `hg::discovery` module provided by the //! Bindings for the `hg::discovery` module provided by the
//! `hg-core` crate. From Python, this will be seen as `rustext.discovery` //! `hg-core` crate. From Python, this will be seen as `rustext.discovery`
//! //!
//! # Classes visible from Python: //! # Classes visible from Python:
//! - [`PartialDiscover`] is the Rust implementation of //! - [`PartialDiscover`] is the Rust implementation of
//! `mercurial.setdiscovery.partialdiscovery`. //! `mercurial.setdiscovery.partialdiscovery`.
use crate::{ use crate::{
cindex::Index, cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
conversion::{py_set, rev_pyiter_collect},
exceptions::GraphError,
}; };
use cpython::{ use cpython::{
ObjectProtocol, PyDict, PyModule, PyObject, PyResult, PyTuple, Python, ObjectProtocol, PyDict, PyModule, PyObject, PyResult, PyTuple, Python,
PythonObject, ToPyObject, PythonObject, ToPyObject,
}; };
use hg::discovery::PartialDiscovery as CorePartialDiscovery; use hg::discovery::PartialDiscovery as CorePartialDiscovery;
use hg::Revision; use hg::Revision;
use std::collections::HashSet;
use std::cell::RefCell; use std::cell::RefCell;
py_class!(pub class PartialDiscovery |py| { py_class!(pub class PartialDiscovery |py| {
data inner: RefCell<Box<CorePartialDiscovery<Index>>>; data inner: RefCell<Box<CorePartialDiscovery<Index>>>;
// `_respectsize` is currently only here to replicate the Python API and // `_respectsize` is currently only here to replicate the Python API and
// will be used in future patches inside methods that are yet to be // will be used in future patches inside methods that are yet to be
let as_dict: PyDict = PyDict::new(py); let as_dict: PyDict = PyDict::new(py);
as_dict.set_item(py, "undecided", as_dict.set_item(py, "undecided",
stats.undecided.map( stats.undecided.map(
|l| l.to_py_object(py).into_object()) |l| l.to_py_object(py).into_object())
.unwrap_or_else(|| py.None()))?; .unwrap_or_else(|| py.None()))?;
Ok(as_dict) Ok(as_dict)
} }
def commonheads(&self) -> PyResult<PyObject> { def commonheads(&self) -> PyResult<HashSet<Revision>> {
py_set( self.inner(py).borrow().common_heads()
py, .map_err(|e| GraphError::pynew(py, e))
&self.inner(py).borrow().common_heads()
.map_err(|e| GraphError::pynew(py, e))?
)
} }
def takefullsample(&self, _headrevs: PyObject, def takefullsample(&self, _headrevs: PyObject,
size: usize) -> PyResult<PyObject> { size: usize) -> PyResult<PyObject> {
let mut inner = self.inner(py).borrow_mut(); let mut inner = self.inner(py).borrow_mut();
let sample = inner.take_full_sample(size) let sample = inner.take_full_sample(size)
.map_err(|e| GraphError::pynew(py, e))?; .map_err(|e| GraphError::pynew(py, e))?;
let as_vec: Vec<PyObject> = sample let as_vec: Vec<PyObject> = sample