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

2019-07-10 Thread kevincox (Kevin Cox) 
kevincox added inline comments.

INLINE COMMENTS

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

Returning a `std::collections::hash_map::Iter` binds you to this 
implementation. I would recommend instead returning `impl Iterator`.

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Raphaël Gomès 
Alphare added a comment.


  In D6593#96417 , @yuja wrote:
  
  >>   Using `keys()` will change the behavior, as I actually need to expose an 
`Iter, 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, u32> {
  >   -self.inner.iter()
  >   +pub fn iter(&self) -> Keys, 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 {
  >   -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 {
  
  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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread yuja (Yuya Nishihara) 
yuja added a comment.


  >   Using `keys()` will change the behavior, as I actually need to expose an 
`Iter, 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, u32> {
-self.inner.iter()
+pub fn iter(&self) -> Keys, 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 {
-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 {

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Yuya Nishihara 
>   Using `keys()` will change the behavior, as I actually need to expose an 
> `Iter, 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, u32> {
-self.inner.iter()
+pub fn iter(&self) -> Keys, 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 {
-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 {

```
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Raphaël Gomès 
Alphare added a comment.


  
  
  > Again,
  >
  > - **contains**() -> inner.contains_key()
  > - iter() -> inner.**keys**()
  >
  > Somewhat similar to HashSet, which is basically a proxy type to
  > HashMap.
  
  `contains()` is my bad, I forgot.
  
  Using `keys()` will change the behavior, as I actually need to expose an 
`Iter, 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Raphaël Gomès 
Closed by commit rHGa80464e85ddd: rust: remove Deref in favor of explicit 
methods (authored by Alphare).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs 
Review".

REPOSITORY
  rHG Mercurial

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

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

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, u32>,
 }
 
-impl Deref for DirsMultiset {
-type Target = HashMap, u32>;
-
-fn deref(&self) -> &Self::Target {
-&self.inner
-}
-}
-
 impl DirsMultiset {
 /// Initializes the multiset from a dirstate or a manifest.
 ///
@@ -132,6 +123,18 @@
 
 Ok(())
 }
+
+pub fn contains_key(&self, key: &[u8]) -> bool {
+self.inner.contains_key(key)
+}
+
+pub fn iter(&self) -> Iter, u32> {
+self.inner.iter()
+}
+
+pub fn len(&self) -> usize {
+self.inner.len()
+}
 }
 
 #[cfg(test)]
@@ -176,8 +179,8 @@
 map.delete_path(b"a/b/")
 );
 
-assert_eq!(2, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&b"a/c".to_vec()).unwrap());
+assert_eq!(2, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(1, *map.inner.get(&b"a/c".to_vec()).unwrap());
 eprintln!("{:?}", map);
 assert_eq!(Ok(()), map.delete_path(b"a/"));
 eprintln!("{:?}", map);
@@ -203,36 +206,36 @@
 let mut map = DirsMultiset::new(DirsIterable::Manifest(vec![]), None);
 
 map.add_path(b"a/");
-assert_eq!(1, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&Vec::new()).unwrap());
+assert_eq!(1, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(1, *map.inner.get(&Vec::new()).unwrap());
 assert_eq!(2, map.len());
 
 // Non directory should be ignored
 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());
 
 // Non directory will still add its base
 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());
 
 // Duplicate path works
 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
 map.add_path(b"a/b/");
-assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&b"a/b".to_vec()).unwrap());
+assert_eq!(4, *map.inner.get(&b"a".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
 map.add_path(b"a/b/c/");
-assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(2, *map.get(&b"a/b".to_vec()).unwrap());
+assert_eq!(4, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(2, *map.inner.get(&b"a/b".to_vec()).unwrap());
 
 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 {
 inner: [("", 2), ("a", 5), ("a/b", 2), ("a/b/c", 1), ("a/c", 1)]



To: Alphare, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread yuja (Yuya Nishihara) 
yuja added a comment.


  > +pub fn contains_key(&self, key: &[u8]) -> bool {
  > +self.inner.contains_key(key)
  > +}
  > +
  > +pub fn iter(&self) -> Iter, u32> {
  > +self.inner.iter()
  > +}
  
  Again,
  
  - **contains**() -> inner.contains_key()
  - iter() -> inner.**keys**()
  
  Somewhat similar to HashSet, which is basically a proxy type to
  HashMap.
  
  I queued this as I think it's strictly better than using Deref, but I expect
  a follow up patch. Thanks.

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Yuya Nishihara 
> +pub fn contains_key(&self, key: &[u8]) -> bool {
> +self.inner.contains_key(key)
> +}
> +
> +pub fn iter(&self) -> Iter, u32> {
> +self.inner.iter()
> +}

Again,

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

Somewhat similar to HashSet, which is basically a proxy type to
HashMap.

I queued this as I think it's strictly better than using Deref, but I expect
a follow up patch. Thanks.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-05 Thread Raphaël Gomès 
Alphare updated this revision to Diff 15771.

REPOSITORY
  rHG Mercurial

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

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

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, u32>,
 }
 
-impl Deref for DirsMultiset {
-type Target = HashMap, u32>;
-
-fn deref(&self) -> &Self::Target {
-&self.inner
-}
-}
-
 impl DirsMultiset {
 /// Initializes the multiset from a dirstate or a manifest.
 ///
@@ -132,6 +123,18 @@
 
 Ok(())
 }
+
+pub fn contains_key(&self, key: &[u8]) -> bool {
+self.inner.contains_key(key)
+}
+
+pub fn iter(&self) -> Iter, u32> {
+self.inner.iter()
+}
+
+pub fn len(&self) -> usize {
+self.inner.len()
+}
 }
 
 #[cfg(test)]
@@ -176,8 +179,8 @@
 map.delete_path(b"a/b/")
 );
 
-assert_eq!(2, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&b"a/c".to_vec()).unwrap());
+assert_eq!(2, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(1, *map.inner.get(&b"a/c".to_vec()).unwrap());
 eprintln!("{:?}", map);
 assert_eq!(Ok(()), map.delete_path(b"a/"));
 eprintln!("{:?}", map);
@@ -203,36 +206,36 @@
 let mut map = DirsMultiset::new(DirsIterable::Manifest(vec![]), None);
 
 map.add_path(b"a/");
-assert_eq!(1, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&Vec::new()).unwrap());
+assert_eq!(1, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(1, *map.inner.get(&Vec::new()).unwrap());
 assert_eq!(2, map.len());
 
 // Non directory should be ignored
 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());
 
 // Non directory will still add its base
 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());
 
 // Duplicate path works
 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
 map.add_path(b"a/b/");
-assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(1, *map.get(&b"a/b".to_vec()).unwrap());
+assert_eq!(4, *map.inner.get(&b"a".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
 map.add_path(b"a/b/c/");
-assert_eq!(4, *map.get(&b"a".to_vec()).unwrap());
-assert_eq!(2, *map.get(&b"a/b".to_vec()).unwrap());
+assert_eq!(4, *map.inner.get(&b"a".to_vec()).unwrap());
+assert_eq!(2, *map.inner.get(&b"a/b".to_vec()).unwrap());
 
 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 {
 inner: [("", 2), ("a", 5), ("a/b", 2), ("a/b/c", 1), ("a/c", 1)]



To: Alphare, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-04 Thread yuja (Yuya Nishihara) 
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, 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-04 Thread Yuya Nishihara 
>  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, 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-04 Thread Raphaël Gomès 
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

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, u32>,
 }
 
-impl Deref for DirsMultiset {
-type Target = HashMap, 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, 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)]



To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2019-07-04 Thread Raphaël Gomès 
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

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, u32>,
 }
 
-impl Deref for DirsMultiset {
-type Target = HashMap, 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, 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)]



To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel