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

login
register
mail settings
Submitter phabricator
Date July 4, 2019, 2:59 p.m.
Message ID <differential-rev-PHID-DREV-oeo57bii6i7uqkf3cgc4-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40745/
State Superseded
Headers show

Comments

phabricator - July 4, 2019, 2:59 p.m.
Alphare created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
Alphare updated this revision to Diff 15740.

REPOSITORY
  rHG Mercurial

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

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

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - July 5, 2019, 1:13 p.m.
Alphare added a comment.


  
  
  > 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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6593/new/

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

To: Alphare, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
phabricator - July 5, 2019, 2:29 p.m.
Alphare added a comment.


  In D6593#96417 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6593/new/

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

To: Alphare, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
phabricator - July 10, 2019, 3:22 p.m.
kevincox added inline comments.

INLINE COMMENTS

> dirs_multiset.rs:131
> +
> +    pub fn iter(&self) -> Iter<Vec<u8>, u32> {
> +        self.inner.iter()

Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would recommend instead returning `impl Iterator<Item=(&[u8], u32)>`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6593/new/

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

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

Patch

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,9 +8,8 @@ 
 //! A multiset of directory names.
 //!
 //! 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::ops::Deref;
 use {DirsIterable, DirstateEntry, DirstateMapError};
 
 #[derive(PartialEq, Debug)]
@@ -18,14 +17,6 @@ 
     inner: HashMap<Vec<u8>, u32>,
 }
 
-impl Deref for DirsMultiset {
-    type Target = HashMap<Vec<u8>, u32>;
-
-    fn deref(&self) -> &Self::Target {
-        &self.inner
-    }
-}
-
 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)
+    }
 }
 
 #[cfg(test)]