D7796: rust-nodemap: input/output primitives

Alphare (Raphaël Gomès) phabricator at mercurial-scm.org
Fri Feb 14 17:26:59 UTC 2020


Alphare added inline comments.

INLINE COMMENTS

> kevincox wrote in nodemap.rs:268
> This doesn't address the primary problem of padding. There is nothing in the code that guarantees that there will be no padding inserted into `Block` (although it is unlikely to ever happen). This is because if there is padding it won't be guaranteed to be initialized and reading uninitialized data is undefined behaviour (not to mention that it will give incorrect results). I would like to add something that guarantees this. The only thing I can think of that will give us this confidence is a check `sizeof::<Block>() == 4 * 16`. I was suggesting the transmute to assert this (since transpose checks the size of its type arguments are the same. However as I originally suggested and as currently written it doesn't do that.
> 
> My current proposal is change `BLOCK_SIZE` to be defined as `4 * 16` (possibly extracting that 16 into a `ELEMENTS_PER_BLOCK`) and keep the current transmute assertion. That paired with some comments explaining why we are doing that will make me confident that there is no padding and we aren't performing any undefined behaviour.

I think that would be sufficient indeed. How about making `Block` into a `[u8; 64]`? It will be less expressive, but this will no longer be needed.

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, durin42
Cc: yuja, Alphare, marmoute, durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list