swym icon indicating copy to clipboard operation
swym copied to clipboard

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

Open Centril opened this issue 6 years ago • 4 comments

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

Centril avatar Feb 27 '19 14:02 Centril

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- avatar Feb 27 '19 18:02 mtak-

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.

mtak- avatar Mar 30 '19 18:03 mtak-

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

Centril avatar Mar 30 '19 18:03 Centril

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.

RalfJung avatar Mar 31 '19 14:03 RalfJung