I noticed a potential problem in the struct evenio::event::EventQueueItem: The documentation for its field event: NonNull<u8> states that When null, ownership of the event has been transferred and no destructor needs to run..
However, a NonNull may never be null, as its documentation clarifies:
/// Unlike `*mut T`, the pointer must always be non-null, even if the pointer
/// is never dereferenced. This is so that enums may use this forbidden value
/// as a discriminant -- `Option<NonNull<T>>` has the same size as `*mut T`.
/// However the pointer may still dangle if it isn't dereferenced.
In the code, I couldn't find event ever being set to a null pointer, nor could I find it being contained in an Option. Still, this is inconsistent in all cases:
If event is never actually null, the documentation should be updated accordingly.
If event is sometimes null, but never put into an Option, it should still have the type *mut u8 in order to uphold the safety requirements of NonNull and to avoid confusion.
If event is sometimes null and put into an Option somewhere, this is undefined behaviour and event should have the type *mut u8.
Alternatively, instead of choosing *mut u8 as event's type and using the null pointer to mark transfer of ownership, it would, in my opinion, be more elegant to simply use an Option<NonNull<u8>>: This would work in exactly the same way (it has the same size as *mut u8, and the same bit pattern - all zeroes - would be used to mark transfer of ownership) and probably even compile to the same LLVM IR, but it would be clearer and more explicit.
I'm pretty new to contributing to open source, so I hope opening this issue is appropriate. I'd be happy to implement a fix for this inconsistency and make a PR.
I noticed a potential problem in the struct
evenio::event::EventQueueItem: The documentation for its fieldevent: NonNull<u8>states thatWhen null, ownership of the event has been transferred and no destructor needs to run.. However, aNonNullmay never be null, as its documentation clarifies:In the code, I couldn't find
eventever being set to a null pointer, nor could I find it being contained in anOption. Still, this is inconsistent in all cases:eventis never actually null, the documentation should be updated accordingly.eventis sometimes null, but never put into anOption, it should still have the type*mut u8in order to uphold the safety requirements ofNonNulland to avoid confusion.eventis sometimes null and put into anOptionsomewhere, this is undefined behaviour andeventshould have the type*mut u8.Alternatively, instead of choosing
*mut u8asevent's type and using the null pointer to mark transfer of ownership, it would, in my opinion, be more elegant to simply use anOption<NonNull<u8>>: This would work in exactly the same way (it has the same size as*mut u8, and the same bit pattern - all zeroes - would be used to mark transfer of ownership) and probably even compile to the same LLVM IR, but it would be clearer and more explicit.I'm pretty new to contributing to open source, so I hope opening this issue is appropriate. I'd be happy to implement a fix for this inconsistency and make a PR.