-
Notifications
You must be signed in to change notification settings - Fork 910
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
Make EventLoopProxy
Sync
#3449
Make EventLoopProxy
Sync
#3449
Conversation
Maybe we should push the |
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.
This is great!
I don't think it hurts to merge this in the meantime. |
tests/sync_object.rs
Outdated
#[allow(dead_code)] | ||
fn is_send<T: 'static + Send>() { | ||
// ensures that `winit::EventLoopProxy` implements `Send` | ||
needs_sync::<winit::event_loop::EventLoopProxy<T>>(); | ||
} |
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.
This should just use needs_sync
, because we test Send
in other test.
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.
This doesn't test if EventLoopProxy
implements Send
, it tests if EventLoopProxy
implement Sync
if T
is Send
.
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.
But you can move the T: Send
in the test itself?
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 think you can use generics on #[test]
. It would be a compile error anyway so we don't actually need #[test]
in the first place.
I think the way it's right now is just fine, but maybe adding a comment would help.
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.
Not sure what I should write as a comment, but I've made the exiting comment text a bit clearer
d17b208
to
bc63612
Compare
Co-authored-by: daxpedda <daxpedda@gmail.com> Closes: #3448
bc63612
to
3d23283
Compare
3d23283
to
358d61c
Compare
Fixes #3448.
CHANGELOG.md
if knowledge of this change could be valuable to users