D7795: rust-nodemap: insert method

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Thu Jan 16 06:02:02 EST 2020


kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> nodemap.rs:268
> +    /// itself because of the mutable borrow taken with the returned `Block`
> +    fn mutable_block(&mut self, idx: usize) -> (usize, &mut Block, usize) {
> +        let ro_blocks = &self.readonly;

It looks like this return value could use some abstraction but maybe its best to wait until there are more users?

> nodemap.rs:273
> +        if idx < ro_len {
> +            // TODO OPTIM I think this makes two copies
> +            self.growable.push(ro_blocks[idx].clone());

I don't quite understand. You have the immutable copy and the mutable copy but that is WAI right?

> nodemap.rs:306
> +        // visit_steps cannot be empty, since we always visit the root block
> +        let (deepest_idx, mut nybble, rev_opt) = visit_steps.pop().unwrap();
> +        let (mut block_idx, mut block, mut glen) =

nybble is very vague. Is this deepest_nibble or something?

> nodemap.rs:504
> +        node_from_hex(&format!("{:0<40}", hex)).unwrap()
> +    }
> +

Should these be `#[cfg(test)]`? We can always remove it later if there is a valid production use but it makes new users think twice.

REPOSITORY
  rHG Mercurial

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

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

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


More information about the Mercurial-devel mailing list