kevincox added inline comments. INLINE COMMENTS
> Alphare wrote in nodemap.rs:268 > @kevincox @durin42: I've changed the code to use the type-level > `ManuallyDrop` instead of `mem::forget`, added the `transmute` to be extra > paranoid and added a comment. 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. > gracinet wrote in nodemap.rs:272 > Not sure to understand what you suggest here, since I'm already using > `Vec::from_raw_parts` in that method. I couldn't find an `into_raw_parts`. > Anyway, the official example > <https://doc.rust-lang.org/std/vec/struct.Vec.html#examples-4> has the > `mem::forget` before `Vec::from_raw_parts`. Would that be sufficient? > > I agree that a leak upon some exceptional-cannot-happen condition is better > than a double free. > > Also, forgot to came back to that one in the latest phab update Ah, is missed that into_raw_parts is nightly-only at the moment. I think the forget is sufficient for now. Maybe leave a TODO to use into_raw_parts once stabilized. 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: Alphare, marmoute, durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
