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