Patchwork D7792: rust-nodemap: abstracting the indexing

login
register
mail settings
Submitter phabricator
Date Jan. 6, 2020, 7:25 p.m.
Message ID <differential-rev-PHID-DREV-e7zefmekdau274u66hv7-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44151/
State Superseded
Headers show

Comments

phabricator - Jan. 6, 2020, 7:25 p.m.
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  In the forthcoming mutable implementation, we'll have to visit
  node trees that are more complex than a single slice, although
  the algorithm will still be expressed in simple indexing terms.
  
  We still refrain using `#[inline]` indications as being
  premature optimizations, but we strongly hope the compiler will
  indeed inline most of the glue.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS




To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Jan. 16, 2020, 10:10 a.m.
kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> nodemap.rs:192
>      ) -> Result<Option<Revision>, NodeMapError> {
> -        let blocks: &[Block] = &*self.readonly;
> -        if blocks.is_empty() {
> +        let len = self.len();
> +        if len == 0 {

I don't think this variable helps readability. I would just repeat `self.len()`.

> nodemap.rs:193
> +        let len = self.len();
> +        if len == 0 {
>              return Ok(None);

I would add a `self.is_empty()` helper. It's good practice for anything that has a `.len()`.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
phabricator - Jan. 27, 2020, 3:53 p.m.
gracinet added inline comments.
gracinet marked 2 inline comments as done.

INLINE COMMENTS

> kevincox wrote in nodemap.rs:193
> I would add a `self.is_empty()` helper. It's good practice for anything that has a `.len()`.

Sure

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -15,6 +15,7 @@ 
 use super::{NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex};
 use std::fmt;
 use std::ops::Deref;
+use std::ops::Index;
 
 #[derive(Debug, PartialEq)]
 pub enum NodeMapError {
@@ -152,6 +153,14 @@ 
     readonly: Box<dyn Deref<Target = [Block]> + Send>,
 }
 
+impl Index<usize> for NodeTree {
+    type Output = Block;
+
+    fn index(&self, i: usize) -> &Block {
+        &self.readonly[i]
+    }
+}
+
 /// Return `None` unless the `Node` for `rev` has given prefix in `index`.
 fn has_prefix_or_none<'p>(
     idx: &impl RevlogIndex,
@@ -170,6 +179,10 @@ 
 }
 
 impl NodeTree {
+    fn len(&self) -> usize {
+        self.readonly.len()
+    }
+
     /// Main working method for `NodeTree` searches
     ///
     /// This partial implementation lacks
@@ -178,14 +191,14 @@ 
         &self,
         prefix: NodePrefixRef<'p>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        let blocks: &[Block] = &*self.readonly;
-        if blocks.is_empty() {
+        let len = self.len();
+        if len == 0 {
             return Ok(None);
         }
-        let mut visit = blocks.len() - 1;
+        let mut visit = len - 1;
         for i in 0..prefix.len() {
             let nybble = prefix.get_nybble(i);
-            match blocks[visit].read(nybble) {
+            match self[visit].read(nybble) {
                 Element::None => return Ok(None),
                 Element::Rev(r) => return Ok(Some(r)),
                 Element::Block(idx) => visit = idx,