Patchwork D10558: rust: Use `&HgPath` instead of `&HgPathBuf` in may APIs

login
register
mail settings
Submitter phabricator
Date May 3, 2021, 10:29 a.m.
Message ID <differential-rev-PHID-DREV-gmlxsoszffs64ccu3cbh-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/48879/
State Superseded
Headers show

Comments

phabricator - May 3, 2021, 10:29 a.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  Getting the former (through `Deref`) is almost the only useful thing one can
  do with the latter anyway. With this changes, API become more flexible for the
  "provider" of these paths which may store something else that Deref’s to HgPath,
  such as `std::borrow::Cow<HgPath>`. Using `Cow` can help reduce memory alloactions
  and copying.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/dirstate.rs
  rust/hg-core/src/dirstate/dirs_multiset.rs
  rust/hg-core/src/dirstate/dirstate_map.rs
  rust/hg-core/src/dirstate/parsers.rs
  rust/hg-core/src/dirstate_tree/dirstate_map.rs
  rust/hg-core/src/dirstate_tree/dispatch.rs
  rust/hg-cpython/src/dirstate/copymap.rs
  rust/hg-cpython/src/dirstate/dirstate_map.rs
  rust/hg-cpython/src/dirstate/dispatch.rs
  rust/hg-cpython/src/dirstate/non_normal_entries.rs

CHANGE DETAILS




To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/dirstate/non_normal_entries.rs b/rust/hg-cpython/src/dirstate/non_normal_entries.rs
--- a/rust/hg-cpython/src/dirstate/non_normal_entries.rs
+++ b/rust/hg-cpython/src/dirstate/non_normal_entries.rs
@@ -12,7 +12,7 @@ 
 };
 
 use crate::dirstate::DirstateMap;
-use hg::utils::hg_path::HgPathBuf;
+use hg::utils::hg_path::HgPath;
 use std::cell::RefCell;
 
 py_class!(pub class NonNormalEntries |py| {
@@ -54,16 +54,13 @@ 
         Ok(true)
     }
 
-    fn translate_key(
-        py: Python,
-        key: &HgPathBuf,
-    ) -> PyResult<Option<PyBytes>> {
+    fn translate_key(py: Python, key: &HgPath) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, key.as_bytes())))
     }
 }
 
 type NonNormalEntriesIter<'a> =
-    Box<dyn Iterator<Item = &'a HgPathBuf> + Send + 'a>;
+    Box<dyn Iterator<Item = &'a HgPath> + Send + 'a>;
 
 py_shared_iterator!(
     NonNormalEntriesIterator,
diff --git a/rust/hg-cpython/src/dirstate/dispatch.rs b/rust/hg-cpython/src/dirstate/dispatch.rs
--- a/rust/hg-cpython/src/dirstate/dispatch.rs
+++ b/rust/hg-cpython/src/dirstate/dispatch.rs
@@ -61,7 +61,7 @@ 
 
     fn non_normal_or_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + '_> {
         self.get_mut().non_normal_or_other_parent_paths()
     }
 
@@ -71,19 +71,19 @@ 
 
     fn iter_non_normal_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         self.get_mut().iter_non_normal_paths()
     }
 
     fn iter_non_normal_paths_panic(
         &self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         self.get().iter_non_normal_paths_panic()
     }
 
     fn iter_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         self.get_mut().iter_other_parent_paths()
     }
 
@@ -141,7 +141,7 @@ 
         self.get().copy_map_contains_key(key)
     }
 
-    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPathBuf> {
+    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPath> {
         self.get().copy_map_get(key)
     }
 
diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -514,13 +514,13 @@ 
     }
     fn translate_key(
         py: Python,
-        res: (&HgPathBuf, &DirstateEntry),
+        res: (&HgPath, &DirstateEntry),
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.0.as_bytes())))
     }
     fn translate_key_value(
         py: Python,
-        res: (&HgPathBuf, &DirstateEntry),
+        res: (&HgPath, &DirstateEntry),
     ) -> PyResult<Option<(PyBytes, PyObject)>> {
         let (f, entry) = res;
         Ok(Some((
diff --git a/rust/hg-cpython/src/dirstate/copymap.rs b/rust/hg-cpython/src/dirstate/copymap.rs
--- a/rust/hg-cpython/src/dirstate/copymap.rs
+++ b/rust/hg-cpython/src/dirstate/copymap.rs
@@ -14,7 +14,8 @@ 
 use std::cell::RefCell;
 
 use crate::dirstate::dirstate_map::DirstateMap;
-use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
+use hg::utils::hg_path::HgPath;
+use hg::CopyMapIter;
 
 py_class!(pub class CopyMap |py| {
     data dirstate_map: DirstateMap;
@@ -87,13 +88,13 @@ 
     }
     fn translate_key(
         py: Python,
-        res: (&HgPathBuf, &HgPathBuf),
+        res: (&HgPath, &HgPath),
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.0.as_bytes())))
     }
     fn translate_key_value(
         py: Python,
-        res: (&HgPathBuf, &HgPathBuf),
+        res: (&HgPath, &HgPath),
     ) -> PyResult<Option<(PyBytes, PyBytes)>> {
         let (k, v) = res;
         Ok(Some((
diff --git a/rust/hg-core/src/dirstate_tree/dispatch.rs b/rust/hg-core/src/dirstate_tree/dispatch.rs
--- a/rust/hg-core/src/dirstate_tree/dispatch.rs
+++ b/rust/hg-core/src/dirstate_tree/dispatch.rs
@@ -47,21 +47,21 @@ 
 
     fn non_normal_or_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + '_>;
+    ) -> Box<dyn Iterator<Item = &HgPath> + '_>;
 
     fn set_non_normal_other_parent_entries(&mut self, force: bool);
 
     fn iter_non_normal_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_>;
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_>;
 
     fn iter_non_normal_paths_panic(
         &self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_>;
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_>;
 
     fn iter_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_>;
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_>;
 
     fn has_tracked_dir(
         &mut self,
@@ -97,7 +97,7 @@ 
 
     fn copy_map_contains_key(&self, key: &HgPath) -> bool;
 
-    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPathBuf>;
+    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPath>;
 
     fn copy_map_remove(&mut self, key: &HgPath) -> Option<HgPathBuf>;
 
@@ -163,10 +163,10 @@ 
 
     fn non_normal_or_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + '_> {
         let (non_normal, other_parent) =
             self.get_non_normal_other_parent_entries();
-        Box::new(non_normal.union(other_parent))
+        Box::new(non_normal.union(other_parent).map(|p| &**p))
     }
 
     fn set_non_normal_other_parent_entries(&mut self, force: bool) {
@@ -175,26 +175,26 @@ 
 
     fn iter_non_normal_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         let (non_normal, _other_parent) =
             self.get_non_normal_other_parent_entries();
-        Box::new(non_normal.iter())
+        Box::new(non_normal.iter().map(|p| &**p))
     }
 
     fn iter_non_normal_paths_panic(
         &self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         let (non_normal, _other_parent) =
             self.get_non_normal_other_parent_entries_panic();
-        Box::new(non_normal.iter())
+        Box::new(non_normal.iter().map(|p| &**p))
     }
 
     fn iter_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         let (_non_normal, other_parent) =
             self.get_non_normal_other_parent_entries();
-        Box::new(other_parent.iter())
+        Box::new(other_parent.iter().map(|p| &**p))
     }
 
     fn has_tracked_dir(
@@ -243,15 +243,15 @@ 
     }
 
     fn copy_map_iter(&self) -> CopyMapIter<'_> {
-        Box::new(self.copy_map.iter())
+        Box::new(self.copy_map.iter().map(|(key, value)| (&**key, &**value)))
     }
 
     fn copy_map_contains_key(&self, key: &HgPath) -> bool {
         self.copy_map.contains_key(key)
     }
 
-    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPathBuf> {
-        self.copy_map.get(key)
+    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPath> {
+        self.copy_map.get(key).map(|p| &**p)
     }
 
     fn copy_map_remove(&mut self, key: &HgPath) -> Option<HgPathBuf> {
@@ -279,6 +279,6 @@ 
     }
 
     fn iter(&self) -> StateMapIter<'_> {
-        Box::new((&**self).iter())
+        Box::new((&**self).iter().map(|(key, value)| (&**key, value)))
     }
 }
diff --git a/rust/hg-core/src/dirstate_tree/dirstate_map.rs b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
--- a/rust/hg-core/src/dirstate_tree/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
@@ -67,7 +67,7 @@ 
 
 /// `(full_path, entry, copy_source)`
 type NodeDataMut<'a> = (
-    &'a WithBasename<HgPathBuf>,
+    &'a HgPath,
     &'a mut Option<DirstateEntry>,
     &'a mut Option<HgPathBuf>,
 );
@@ -248,8 +248,7 @@ 
 
     fn iter_nodes<'a>(
         &'a self,
-    ) -> impl Iterator<Item = (&'a WithBasename<HgPathBuf>, &'a Node)> + 'a
-    {
+    ) -> impl Iterator<Item = (&'a HgPath, &'a Node)> + 'a {
         // Depth first tree traversal.
         //
         // If we could afford internal iteration and recursion,
@@ -276,6 +275,7 @@ 
                 // Pseudo-recursion
                 let new_iter = child_node.children.iter();
                 let old_iter = std::mem::replace(&mut iter, new_iter);
+                let key = &**key.full_path();
                 stack.push((key, child_node, old_iter));
             }
             // Found the end of a `children.iter()` iterator.
@@ -307,8 +307,11 @@ 
         std::iter::from_fn(move || {
             while let Some((key, child_node)) = iter.next() {
                 // Pseudo-recursion
-                let data =
-                    (key, &mut child_node.entry, &mut child_node.copy_source);
+                let data = (
+                    &**key.full_path(),
+                    &mut child_node.entry,
+                    &mut child_node.copy_source,
+                );
                 let new_iter = child_node.children.iter_mut();
                 let old_iter = std::mem::replace(&mut iter, new_iter);
                 stack.push((data, old_iter));
@@ -421,14 +424,14 @@ 
 
     fn non_normal_or_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + '_> {
         Box::new(self.iter_nodes().filter_map(|(path, node)| {
             node.entry
                 .as_ref()
                 .filter(|entry| {
                     entry.is_non_normal() || entry.is_from_other_parent()
                 })
-                .map(|_| path.full_path())
+                .map(|_| path)
         }))
     }
 
@@ -439,29 +442,29 @@ 
 
     fn iter_non_normal_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         self.iter_non_normal_paths_panic()
     }
 
     fn iter_non_normal_paths_panic(
         &self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         Box::new(self.iter_nodes().filter_map(|(path, node)| {
             node.entry
                 .as_ref()
                 .filter(|entry| entry.is_non_normal())
-                .map(|_| path.full_path())
+                .map(|_| path)
         }))
     }
 
     fn iter_other_parent_paths(
         &mut self,
-    ) -> Box<dyn Iterator<Item = &HgPathBuf> + Send + '_> {
+    ) -> Box<dyn Iterator<Item = &HgPath> + Send + '_> {
         Box::new(self.iter_nodes().filter_map(|(path, node)| {
             node.entry
                 .as_ref()
                 .filter(|entry| entry.is_from_other_parent())
-                .map(|_| path.full_path())
+                .map(|_| path)
         }))
     }
 
@@ -502,8 +505,8 @@ 
         for (path, node) in self.iter_nodes() {
             if node.entry.is_some() {
                 size += packed_entry_size(
-                    path.full_path(),
-                    node.copy_source.as_ref(),
+                    path,
+                    node.copy_source.as_ref().map(|p| &**p),
                 )
             }
         }
@@ -516,9 +519,9 @@ 
             if let Some(entry) = opt_entry {
                 clear_ambiguous_mtime(entry, now);
                 pack_entry(
-                    path.full_path(),
+                    path,
                     entry,
-                    copy_source.as_ref(),
+                    copy_source.as_ref().map(|p| &**p),
                     &mut packed,
                 );
             }
@@ -557,7 +560,7 @@ 
         Box::new(self.iter_nodes().filter_map(|(path, node)| {
             node.copy_source
                 .as_ref()
-                .map(|copy_source| (path.full_path(), copy_source))
+                .map(|copy_source| (path, &**copy_source))
         }))
     }
 
@@ -569,8 +572,8 @@ 
         }
     }
 
-    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPathBuf> {
-        self.get_node(key)?.copy_source.as_ref()
+    fn copy_map_get(&self, key: &HgPath) -> Option<&HgPath> {
+        self.get_node(key)?.copy_source.as_ref().map(|p| &**p)
     }
 
     fn copy_map_remove(&mut self, key: &HgPath) -> Option<HgPathBuf> {
@@ -609,7 +612,7 @@ 
 
     fn iter(&self) -> StateMapIter<'_> {
         Box::new(self.iter_nodes().filter_map(|(path, node)| {
-            node.entry.as_ref().map(|entry| (path.full_path(), entry))
+            node.entry.as_ref().map(|entry| (path, entry))
         }))
     }
 }
diff --git a/rust/hg-core/src/dirstate/parsers.rs b/rust/hg-core/src/dirstate/parsers.rs
--- a/rust/hg-core/src/dirstate/parsers.rs
+++ b/rust/hg-core/src/dirstate/parsers.rs
@@ -4,7 +4,7 @@ 
 // GNU General Public License version 2 or any later version.
 
 use crate::errors::HgError;
-use crate::utils::hg_path::{HgPath, HgPathBuf};
+use crate::utils::hg_path::HgPath;
 use crate::{
     dirstate::{CopyMap, EntryState, RawEntry, StateMap},
     DirstateEntry, DirstateParents,
@@ -83,8 +83,8 @@ 
 }
 
 fn packed_filename_and_copy_source_size(
-    filename: &HgPathBuf,
-    copy_source: Option<&HgPathBuf>,
+    filename: &HgPath,
+    copy_source: Option<&HgPath>,
 ) -> usize {
     filename.len()
         + if let Some(source) = copy_source {
@@ -95,17 +95,17 @@ 
 }
 
 pub fn packed_entry_size(
-    filename: &HgPathBuf,
-    copy_source: Option<&HgPathBuf>,
+    filename: &HgPath,
+    copy_source: Option<&HgPath>,
 ) -> usize {
     MIN_ENTRY_SIZE
         + packed_filename_and_copy_source_size(filename, copy_source)
 }
 
 pub fn pack_entry(
-    filename: &HgPathBuf,
+    filename: &HgPath,
     entry: &DirstateEntry,
-    copy_source: Option<&HgPathBuf>,
+    copy_source: Option<&HgPath>,
     packed: &mut Vec<u8>,
 ) {
     let length = packed_filename_and_copy_source_size(filename, copy_source);
@@ -159,7 +159,7 @@ 
     let expected_size: usize = state_map
         .iter()
         .map(|(filename, _)| {
-            packed_entry_size(filename, copy_map.get(filename))
+            packed_entry_size(filename, copy_map.get(filename).map(|p| &**p))
         })
         .sum();
     let expected_size = expected_size + PARENT_SIZE * 2;
@@ -171,7 +171,12 @@ 
 
     for (filename, entry) in state_map.iter_mut() {
         clear_ambiguous_mtime(entry, now);
-        pack_entry(filename, entry, copy_map.get(filename), &mut packed)
+        pack_entry(
+            filename,
+            entry,
+            copy_map.get(filename).map(|p| &**p),
+            &mut packed,
+        )
     }
 
     if packed.len() != expected_size {
diff --git a/rust/hg-core/src/dirstate/dirstate_map.rs b/rust/hg-core/src/dirstate/dirstate_map.rs
--- a/rust/hg-core/src/dirstate/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate/dirstate_map.rs
@@ -260,7 +260,7 @@ 
     pub fn set_dirs(&mut self) -> Result<(), DirstateMapError> {
         if self.dirs.is_none() {
             self.dirs = Some(DirsMultiset::from_dirstate(
-                &self.state_map,
+                self.state_map.iter(),
                 Some(EntryState::Removed),
             )?);
         }
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
@@ -30,17 +30,22 @@ 
     /// Initializes the multiset from a dirstate.
     ///
     /// If `skip_state` is provided, skips dirstate entries with equal state.
-    pub fn from_dirstate<'a>(
-        dirstate: impl IntoIterator<Item = (&'a HgPathBuf, &'a DirstateEntry)>,
+    pub fn from_dirstate<'a, I, P>(
+        dirstate: I,
         skip_state: Option<EntryState>,
-    ) -> Result<Self, DirstateMapError> {
+    ) -> Result<Self, DirstateMapError>
+    where
+        I: IntoIterator<Item = (P, &'a DirstateEntry)>,
+        P: AsRef<HgPath>,
+    {
         let mut multiset = DirsMultiset {
             inner: FastHashMap::default(),
         };
-        for (filename, DirstateEntry { state, .. }) in dirstate {
+        for (filename, entry) in dirstate {
+            let filename = filename.as_ref();
             // This `if` is optimized out of the loop
             if let Some(skip) = skip_state {
-                if skip != *state {
+                if skip != entry.state {
                     multiset.add_path(filename)?;
                 }
             } else {
diff --git a/rust/hg-core/src/dirstate.rs b/rust/hg-core/src/dirstate.rs
--- a/rust/hg-core/src/dirstate.rs
+++ b/rust/hg-core/src/dirstate.rs
@@ -7,7 +7,8 @@ 
 
 use crate::errors::HgError;
 use crate::revlog::Node;
-use crate::{utils::hg_path::HgPathBuf, FastHashMap};
+use crate::utils::hg_path::{HgPath, HgPathBuf};
+use crate::FastHashMap;
 use bytes_cast::{unaligned, BytesCast};
 use std::convert::TryFrom;
 
@@ -76,11 +77,11 @@ 
 
 pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>;
 pub type StateMapIter<'a> =
-    Box<dyn Iterator<Item = (&'a HgPathBuf, &'a DirstateEntry)> + Send + 'a>;
+    Box<dyn Iterator<Item = (&'a HgPath, &'a DirstateEntry)> + Send + 'a>;
 
 pub type CopyMap = FastHashMap<HgPathBuf, HgPathBuf>;
 pub type CopyMapIter<'a> =
-    Box<dyn Iterator<Item = (&'a HgPathBuf, &'a HgPathBuf)> + Send + 'a>;
+    Box<dyn Iterator<Item = (&'a HgPath, &'a HgPath)> + Send + 'a>;
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub enum EntryState {