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

login
register
mail settings
Submitter phabricator
Date July 4, 2019, 3 p.m.
Message ID <b832118416ecb0ef87a5e6d48d1399ca@localhost.localdomain>
Download mbox | patch
Permalink /patch/40754/
State Not Applicable
Headers show

Comments

phabricator - July 4, 2019, 3 p.m.
Alphare updated this revision to Diff 15740.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D6593?vs=15729&id=15740

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

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
Yuya Nishihara - July 5, 2019, 12:10 a.m.
>  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.
phabricator - July 5, 2019, 12:13 a.m.
yuja added a comment.


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

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)]