Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency in EventQueueItem #77

Open
AsterixxxGallier opened this issue Aug 16, 2024 · 0 comments
Open

Inconsistency in EventQueueItem #77

AsterixxxGallier opened this issue Aug 16, 2024 · 0 comments

Comments

@AsterixxxGallier
Copy link

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.

@AsterixxxGallier AsterixxxGallier changed the title Inconsistency in evenio::event::EventQueueItem Inconsistency in EventQueueItem Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant