search

mtak- / swym

Efficient transactional memory in rust.
MIT License
209 stars 6 forks source link

Possible improvement: justifying uses of `unsafe { .. }` #3

Open Centril opened 5 years ago

Centril commented 5 years ago

Hi there; Interesting library!

It seems generally well documented from a user's perspective. However, I found the amount of comments justifying uses of unsafe { .. } and unsafe impl to be lacking. I think it could help everyone (including future you...) to document why parts of the library is sound.

As an example, I found it unclear why https://docs.rs/swym/0.1.0-preview/src/swym/tcell.rs.html#197-210 is sound. In particular, I conflated Borrow with the standard library's trait and didn't spot the exclusion of uninhabited types T (if you didn't rule those out it might have been unsound).

Best wishes // Centril

mtak- commented 5 years ago

Thanks for the ping, and issue! There's a lot of unsafe in swym, and some of it is very subtle. My focus right now is commenting code, to make contribution easier. I will also try to justify unsafe as best I can.

mtak- commented 5 years ago

A lot of work has recently been done here. The alloc submodule has the most work left to do on documenting unsafe.

Additionally I am not sure if the smuggling of usizes through references in read is UB in rust or not. In c++ the reinterpret_cast rules allow pointer -> usize -> pointer, but forbid usize -> pointer -> usize.

Centril commented 5 years ago

cc @RalfJung re. the implementation ^-- there.

RalfJung commented 5 years ago

This is a reference to a 1-aligned ZST? Then I think you should be fine as long as you make sure this usize is never 0.