D6628: rust-parsers: switch to parse/pack_dirstate to mutate-on-loop

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Mon Jul 15 12:24:50 EDT 2019


kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> mod.rs:16
> +    pub p1: Vec<u8>,
> +    pub p2: Vec<u8>,
>  }

If these are always 20 bytes long you are best to use `[u8; 20]`. That way there will be no indirection. And since `Vec` is 3 words it will have a bigger immediate size on 64bit platforms.

> parsers.rs:81
>      parents: DirstateParents,
> -    now: i32,
> -) -> Result<(Vec<u8>, DirstateVec), DirstatePackError> {
> +    now: Duration,
> +) -> Result<Vec<u8>, DirstatePackError> {

The "proper" solution is SystemTime <https://doc.rust-lang.org/std/time/struct.SystemTime.html> however I will admit that it is a bit more awkward to use. However if you are going to use Duration please document that it is the Duration since the Unix Epoch.

> parsers.rs:87
>  
> -    let expected_size: usize = dirstate_vec
> +    let now = now.as_secs() as i32;
> +

Would it be best to panic on overflow? `now.as_secs().try_into::<i32>().expect("time overflow")`.

REPOSITORY
  rHG Mercurial

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

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

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


More information about the Mercurial-devel mailing list