NULLx76 / ringbuffer

A fixed-size circular buffer written in Rust.
https://crates.io/crates/ringbuffer
MIT License
95 stars 20 forks source link

Remove default bound from ringbuffer trait using MaybeUninit #32

Closed vgel closed 4 years ago

vgel commented 4 years ago

Fixes #13

Use MaybeUninit in ConstGenericRingBuffer / GenericRingBuffer. Adds new unsafe methods get_unchecked / get_unchecked_mut, and refactors the impl_ringbuffer! macro to use those methods instead of directly accessing the buffer, so we can munge the MaybeUninit for the impls that need it. These methods should maybe be part of the Ringbuffer trait? Right now they're non-exported impls that the macro can call by virtue of defining methods in the struct anyways.

vgel commented 4 years ago

Would appreciate some extra eyes on my unsafe use BTW -- I don't think I did anything monumentally dumb but you never know :sweat_smile:

jdonszelmann commented 4 years ago

Dear Theia. Thanks for the great contribution! I was just talking to Victor privately and said: "nah the code is unsound" but actually it is not. You did the right thing by placing get_unchecked outside of the trait. Placing them in the trait would create the possibility to get two mutable references to the same memory. (see #25). It'd be slightly harder now because iter_mut is implemented differently, but it's still possible. I don't like them outside of the trait, nor do I like them in the trait. Outside is safer so I'd say it's okay like this.

This is exactly how I wanted to implement it, but you did it first and that's awesome! Could you maybe add either a comment to the macro explaining you need to implement get_unchecked(_mut) for it to work, or alternatively have the names of those two functions be parameters to the macro so you could even rename them (plus it'd be clear that by using the macro you need those functions)

We'll soon merge #31 (even though it's not multithreaded yet, there are many other things we want from that branch) so this will be a fun merge conflict... :P Especially because in that branch get becomes moving, and we mem::replace the element with Default::default(). So some dropping shenanigans will need to be added.

vgel commented 4 years ago

Thanks for the review! Made the changes.

Not sure how to approach integrating with that other PR, hopefully they don't conflict too badly :)

jdonszelmann commented 4 years ago

Don't worry about integrating, we'll take care of it :)

NULLx76 commented 4 years ago

Thanks for the PR! Looks good now, merged :)