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