D7788: rust-node: binary Node and conversion utilities

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Thu Jan 16 05:50:22 EST 2020


kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> node.rs:14
> +/// Binary revisions SHA
> +pub type Node = [u8; 20];
> +

I would recommend making this a proper type because almost no one should be poking at the bytes of the hash.

  struct Node([u8; 20]);

You can make the field `pub` if necessary.

> node.rs:31
> +    for i in 0..20 {
> +        node[i] = u8::from_str_radix(&hex[i * 2..i * 2 + 2], 16)?
> +    }

I find progressing through the string easier to understand than this slicing. WDYT.

  for byte in node.iter_mut() {
      *byte = u8::from_str_radix(&hex[..2], 16)?;
      hex = &hex[2..];
  }

> node.rs:39
> +    as_vec.join("")
> +}
> +

If we want to avoid a number of allocations we can do:

  pub fn node_to_hex(n: &Node) -> String {
      let mut r = String::with_capacity(n.len() * 2);
      for byte in n {
          write!(r, "{:02x}", byte).unwrap();
      }
      debug_assert_eq!(r.len(), n.len() * 2);
      r
  }

The generated code for `write!()` doesn't look great but if hex printing shows up in benchmarks it would be trivial to write a custom hex-formatter.

REPOSITORY
  rHG Mercurial

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

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

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


More information about the Mercurial-devel mailing list