This is an archive of the discontinued Mercurial Phabricator instance.

impl DirsMultiset {
/// Initializes the multiset from a dirstate or a manifest.
///
@@ -132,6 +123,22 @@
    Ok(())
}
+
+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }
+
+ pub fn len(&self) -> usize {
+ self.inner.len()
+ }
+
+ pub fn get(&self, key: &[u8]) -> Option<&u32> {
+ self.inner.get(key)
+ }

Maybe these can be:

pub contains() -> inner.contains_key()
pub iter() -> inner.keys()
pub len() -> inner.len()
(not pub) get() -> inner.get()

The point is that DirsMultiset should be more like a set regarding public API.

get() -> inner.get() can be removed if we rewrite the tests to peek
inner.get(), but I don't care much about that.

Alphare updated this revision to Diff 15771.Jul 5 2019, 4:34 AM

+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }

Again,

contains() -> inner.contains_key()
iter() -> inner.keys()

Somewhat similar to HashSet, which is basically a proxy type to
HashMap<T, ()>.

I queued this as I think it's strictly better than using Deref, but I expect
a follow up patch. Thanks.

Alphare added a commit: rHGa80464e85ddd: rust: remove Deref in favor of explicit methods.Jul 5 2019, 8:51 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHGa80464e85ddd: rust: remove Deref in favor of explicit methods (authored by Alphare). · Explain Why

This revision was automatically updated to reflect the committed changes.

Again,

contains() -> inner.contains_key()

iter() -> inner.keys()

Somewhat similar to HashSet, which is basically a proxy type to
HashMap<T, ()>.

contains() is my bad, I forgot.

Using keys() will change the behavior, as I actually need to expose an Iter<Vec<u8>, u32>. Is your issue with the naming?

Using `keys()` will change the behavior, as I actually need to expose an `Iter<Vec<u8>, u32>`. Is your issue with the naming?

No. My point is that the refcount value is an implementation detail.
Do you have some plan to use the refcount value?

diff --git a/rust/hg-core/src/dirstate/dirs_multiset.rs b/rust/hg-core/src/dirstate/dirs_multiset.rs
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -8,7 +8,7 @@
 //! A multiset of directory names.
 //!
 //! Used to counts the references to directories in a manifest or dirstate.
-use std::collections::hash_map::{Entry, Iter};
+use std::collections::hash_map::{Entry, Keys};
 use std::collections::HashMap;
 use {DirsIterable, DirstateEntry, DirstateMapError};
 
@@ -128,8 +128,8 @@ impl DirsMultiset {
         self.inner.contains_key(key)
     }
 
-    pub fn iter(&self) -> Iter<Vec<u8>, u32> {
-        self.inner.iter()
+    pub fn iter(&self) -> Keys<Vec<u8>, u32> {
+        self.inner.keys()
     }
 
     pub fn len(&self) -> usize {
diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
--- a/rust/hg-cpython/src/dirstate.rs
+++ b/rust/hg-cpython/src/dirstate.rs
@@ -271,20 +271,9 @@ py_class!(pub class Dirs |py| {
     // of having it work to continue working on the rest of the module
     // hopefully bypassing Python entirely pretty soon.
     def __iter__(&self) -> PyResult<PyObject> {
-        let dict = PyDict::new(py);
-
-        for (key, value) in self.dirs_map(py).borrow().iter() {
-            dict.set_item(
-                py,
-                PyBytes::new(py, &key[..]),
-                value.to_py_object(py),
-            )?;
-        }
-
-        let locals = PyDict::new(py);
-        locals.set_item(py, "obj", dict)?;
-
-        py.eval("iter(obj)", None, Some(&locals))
+        // maybe there would be a better way to cast objects back and forth?
+        let dirs: Vec<_> = self.dirs_map(py).borrow().iter().map(|d| PyBytes::new(py, d)).collect();
+        dirs.to_py_object(py).into_object().iter(py).map(|o| o.into_object())
     }
 
     def __contains__(&self, item: PyObject) -> PyResult<bool> {

In D6593#96417, @yuja wrote:

Using `keys()` will change the behavior, as I actually need to expose an `Iter<Vec<u8>, u32>`. Is your issue with the naming?

No. My point is that the refcount value is an implementation detail.
Do you have some plan to use the refcount value?

diff --git a/rust/hg-core/src/dirstate/dirs_multiset.rs b/rust/hg-core/src/dirstate/dirs_multiset.rs
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -8,7 +8,7 @@
 //! A multiset of directory names.
 //!
 //! Used to counts the references to directories in a manifest or dirstate.
-use std::collections::hash_map::{Entry, Iter};
+use std::collections::hash_map::{Entry, Keys};
 use std::collections::HashMap;
 use {DirsIterable, DirstateEntry, DirstateMapError};
@@ -128,8 +128,8 @@ impl DirsMultiset {
         self.inner.contains_key(key)
     }
-    pub fn iter(&self) -> Iter<Vec<u8>, u32> {
-        self.inner.iter()
+    pub fn iter(&self) -> Keys<Vec<u8>, u32> {
+        self.inner.keys()
     }
     pub fn len(&self) -> usize {
diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
--- a/rust/hg-cpython/src/dirstate.rs
+++ b/rust/hg-cpython/src/dirstate.rs
@@ -271,20 +271,9 @@ py_class!(pub class Dirs |py| {
     // of having it work to continue working on the rest of the module
     // hopefully bypassing Python entirely pretty soon.
     def __iter__(&self) -> PyResult<PyObject> {
-        let dict = PyDict::new(py);
-
-        for (key, value) in self.dirs_map(py).borrow().iter() {
-            dict.set_item(
-                py,
-                PyBytes::new(py, &key[..]),
-                value.to_py_object(py),
-            )?;
-        }
-
-        let locals = PyDict::new(py);
-        locals.set_item(py, "obj", dict)?;
-
-        py.eval("iter(obj)", None, Some(&locals))
+        // maybe there would be a better way to cast objects back and forth?
+        let dirs: Vec<_> = self.dirs_map(py).borrow().iter().map(|d| PyBytes::new(py, d)).collect();
+        dirs.to_py_object(py).into_object().iter(py).map(|o| o.into_object())
     }
     def __contains__(&self, item: PyObject) -> PyResult<bool> {

I guess I was trying too much to stick to the exact behavior of the Python implementation, but the refcount is indeed not public, that makes sense, thanks.

Regarding your inline comment, my RFC for dirstatemap contains iterator sharing between Rust and Python, I would be very interested in what you would have to say about it.

kevincox added inline comments.Jul 10 2019, 11:22 AM

rust/hg-core/src/dirstate/dirs_multiset.rs
131	Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would recommend instead returning `impl Iterator<Item=(&[u8], u32)>`.

Revision Contents
Changeset List

			Path	Packages
M			rust/hg-core/src/dirstate/dirs_multiset.rs (47 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	15729		Jul 1 2019, 9:19 AM	★	★
Diff 2	15740		Jul 2 2019, 11:45 AM	★	★
Diff 3	15771		Jul 5 2019, 4:34 AM	★	★
Diff 4	15774	rHGa80464e85ddd27e1e8e9209bbf597724214a23de	Jul 1 2019, 9:07 AM	★	★

Status	Author	Revision
Closed	Alphare	D6593 rust-minor-fixes: remove Deref in favor of explicit methods
Closed	Alphare	D6592 rust-minor-fixes: simplify overly complicated expression
Closed	Alphare	D6591 rust-minor-fixes: run rfmt on all hg-core/hg-cpython code

Diff 15774

rust/hg-core/src/dirstate/dirs_multiset.rs

	// dirs_multiset.rs			// dirs_multiset.rs
	//			//
	// Copyright 2019 Raphaël Gomès <rgomes@octobus.net>			// Copyright 2019 Raphaël Gomès <rgomes@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.

	//! A multiset of directory names.			//! A multiset of directory names.
	//!			//!
	//! Used to counts the references to directories in a manifest or dirstate.			//! Used to counts the references to directories in a manifest or dirstate.
	use std::collections::hash_map::Entry;			use std::collections::hash_map::{Entry, Iter};
	use std::collections::HashMap;			use std::collections::HashMap;
	use std::ops::Deref;
	use {DirsIterable, DirstateEntry, DirstateMapError};			use {DirsIterable, DirstateEntry, DirstateMapError};

	#[derive(PartialEq, Debug)]			#[derive(PartialEq, Debug)]
	pub struct DirsMultiset {			pub struct DirsMultiset {
	inner: HashMap<Vec<u8>, u32>,			inner: HashMap<Vec<u8>, u32>,
	}			}

	impl Deref for DirsMultiset {
	type Target = HashMap<Vec<u8>, u32>;

	fn deref(&self) -> &Self::Target {
	&self.inner
	}
	}

	impl DirsMultiset {			impl DirsMultiset {
	/// Initializes the multiset from a dirstate or a manifest.			/// Initializes the multiset from a dirstate or a manifest.
	///			///
	/// If `skip_state` is provided, skips dirstate entries with equal state.			/// If `skip_state` is provided, skips dirstate entries with equal state.
	pub fn new(iterable: DirsIterable, skip_state: Option<i8>) -> Self {			pub fn new(iterable: DirsIterable, skip_state: Option<i8>) -> Self {
	let mut multiset = DirsMultiset {			let mut multiset = DirsMultiset {
	inner: HashMap::new(),			inner: HashMap::new(),
	};			};
	pos = subpath.len();			pos = subpath.len();
	if pos == 0 {			if pos == 0 {
	break;			break;
	}			}
	}			}

	Ok(())			Ok(())
	}			}

				pub fn contains_key(&self, key: &[u8]) -> bool {
				self.inner.contains_key(key)
				}

				pub fn iter(&self) -> Iter<Vec<u8>, u32> {
				kevincoxUnsubmitted Not Done Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would recommend instead returning `impl Iterator<Item=(&[u8], u32)>`. kevincox: Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would…
				self.inner.iter()
				}

				pub fn len(&self) -> usize {
				self.inner.len()
				}
	}			}

	#[cfg(test)]			#[cfg(test)]
	mod tests {			mod tests {
	use super::*;			use super::*;

	#[test]			#[test]
	fn test_delete_path_path_not_found() {			fn test_delete_path_path_not_found() {

	assert_eq!(Ok(()), map.delete_path(b"a/b/"));			assert_eq!(Ok(()), map.delete_path(b"a/b/"));
	assert_eq!(Ok(()), map.delete_path(b"a/b/"));			assert_eq!(Ok(()), map.delete_path(b"a/b/"));
	assert_eq!(			assert_eq!(
	Err(DirstateMapError::PathNotFound(b"a/b/".to_vec())),			Err(DirstateMapError::PathNotFound(b"a/b/".to_vec())),
	map.delete_path(b"a/b/")			map.delete_path(b"a/b/")
	);			);

	assert_eq!(2, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(2, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(1, *map.get(&b"a/c".to_vec()).unwrap());			assert_eq!(1, *map.inner.get(&b"a/c".to_vec()).unwrap());
	eprintln!("{:?}", map);			eprintln!("{:?}", map);
	assert_eq!(Ok(()), map.delete_path(b"a/"));			assert_eq!(Ok(()), map.delete_path(b"a/"));
	eprintln!("{:?}", map);			eprintln!("{:?}", map);

	assert_eq!(Ok(()), map.delete_path(b"a/c/"));			assert_eq!(Ok(()), map.delete_path(b"a/c/"));
	assert_eq!(			assert_eq!(
	Err(DirstateMapError::PathNotFound(b"a/c/".to_vec())),			Err(DirstateMapError::PathNotFound(b"a/c/".to_vec())),
	map.delete_path(b"a/c/")			map.delete_path(b"a/c/")
	assert_eq!(1, map.len());			assert_eq!(1, map.len());
	}			}

	#[test]			#[test]
	fn test_add_path_successful() {			fn test_add_path_successful() {
	let mut map = DirsMultiset::new(DirsIterable::Manifest(vec![]), None);			let mut map = DirsMultiset::new(DirsIterable::Manifest(vec![]), None);

	map.add_path(b"a/");			map.add_path(b"a/");
	assert_eq!(1, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(1, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(1, *map.get(&Vec::new()).unwrap());			assert_eq!(1, *map.inner.get(&Vec::new()).unwrap());
	assert_eq!(2, map.len());			assert_eq!(2, map.len());

	// Non directory should be ignored			// Non directory should be ignored
	map.add_path(b"a");			map.add_path(b"a");
	assert_eq!(1, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(1, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(2, map.len());			assert_eq!(2, map.len());

	// Non directory will still add its base			// Non directory will still add its base
	map.add_path(b"a/b");			map.add_path(b"a/b");
	assert_eq!(2, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(2, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(2, map.len());			assert_eq!(2, map.len());

	// Duplicate path works			// Duplicate path works
	map.add_path(b"a/");			map.add_path(b"a/");
	assert_eq!(3, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(3, *map.inner.get(&b"a".to_vec()).unwrap());

	// Nested dir adds to its base			// Nested dir adds to its base
	map.add_path(b"a/b/");			map.add_path(b"a/b/");
	assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(4, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(1, *map.get(&b"a/b".to_vec()).unwrap());			assert_eq!(1, *map.inner.get(&b"a/b".to_vec()).unwrap());

	// but not its base's base, because it already existed			// but not its base's base, because it already existed
	map.add_path(b"a/b/c/");			map.add_path(b"a/b/c/");
	assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());			assert_eq!(4, *map.inner.get(&b"a".to_vec()).unwrap());
	assert_eq!(2, *map.get(&b"a/b".to_vec()).unwrap());			assert_eq!(2, *map.inner.get(&b"a/b".to_vec()).unwrap());

	map.add_path(b"a/c/");			map.add_path(b"a/c/");
	assert_eq!(1, *map.get(&b"a/c".to_vec()).unwrap());			assert_eq!(1, *map.inner.get(&b"a/c".to_vec()).unwrap());

	let expected = DirsMultiset {			let expected = DirsMultiset {
	inner: [("", 2), ("a", 5), ("a/b", 2), ("a/b/c", 1), ("a/c", 1)]			inner: [("", 2), ("a", 5), ("a/b", 2), ("a/b/c", 1), ("a/c", 1)]
	.iter()			.iter()
	.map(\|(k, v)\| (k.as_bytes().to_vec(), *v))			.map(\|(k, v)\| (k.as_bytes().to_vec(), *v))
	.collect(),			.collect(),
	};			};
	assert_eq!(map, expected);			assert_eq!(map, expected);

This is an archive of the discontinued Mercurial Phabricator instance.

rust-minor-fixes: remove Deref in favor of explicit methodsClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 15774

rust/hg-core/src/dirstate/dirs_multiset.rs

rust-minor-fixes: remove Deref in favor of explicit methods
ClosedPublic

Revision Contents
Changeset List