-
Notifications
You must be signed in to change notification settings - Fork 68
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
chore: prefer usize::MAX to std::usize::MAX #157
Conversation
Signed-off-by: tison <[email protected]>
src/lib.rs
Outdated
unsafe impl Send for Event {} | ||
unsafe impl Sync for Event {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the additional unsafe
here. Can we just add a test to make sure the Event
struct is Send
and Sync
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a test
Do you mean that we ensure the auto trait happens by testing? What's the difference to this patch that let the compiler do the "test"?
These two lines are automatically done if it meets the auto trait conditions. We declare them explicitly for testing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'm pushing a commit now - this seems a style preference :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have it in _assert_send_and_sync
.
Then I'll convert this patch as a trivial code tidy one.
Signed-off-by: tison <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Per #140 (comment)
Hopefully, we can move
Event
s free. Open for comments :D