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