D7796: rust-nodemap: input/output primitives

[kevincox (Kevin Cox)]

This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.

INLINE COMMENTS

> nodemap.rs:268
> +            Vec::from_raw_parts(
> +                vec.as_ptr() as *mut u8,
> +                vec.len() * BLOCK_SIZE,

This makes me uncomfortable. There is really no guarantee that there is no padding around `Block` or `RawElement`. If there is any padding this is UB. I would much prefer one of:

1. Store growable as a set of bytes and convert on get/set.
2. Add a method that outputs the bytes to any `Write`. In theory this will be slower but it is probably immaterial (especially if there is no padding).

> nodemap.rs:272
> +            )
> +        };
> +        mem::forget(vec);

If someone panics between the `from_raw_parts` and `mem::forget` this is a double free. Right now I think this can't happen but it is completely possible that the code changes between then and now. I would prefer using `Vec::from_raw_parts` to make sure that there is only one `Vec` alive with that pointer at a time. This risks a leak in the face of panic but I think that is much better.

> nodemap.rs:433
> +        amount: usize,
> +    ) -> Self {
> +        assert!(buffer.len() >= offset + amount);

This is a weird API. Why does new take a buffer and an offset? Can it take either a slice or just remove the offset parameter and always have the buffer start at 0? It is very strange to be passing in data that we own but isn't ever used.

REPOSITORY
  rHG Mercurial

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

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

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