( )⚙ D6593 rust-minor-fixes: remove Deref in favor of explicit methods

This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by Alphare on Jul 1 2019, 9:19 AM.

Details

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Alphare created this revision.Jul 1 2019, 9:19 AM
Alphare updated this revision to Diff 15740.Jul 2 2019, 11:45 AM
yuja added a subscriber: yuja.Jul 4 2019, 8:13 PM

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
yuja added a comment.Jul 5 2019, 8:51 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.

This revision was not accepted when it landed; it landed in state Needs Review.
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?

yuja added a comment.Jul 5 2019, 10:23 AM
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)>.