-
Notifications
You must be signed in to change notification settings - Fork 394
Remove leaky synchronisation objects. #4411
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
Remove leaky synchronisation objects. #4411
Conversation
@rustbot ready |
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 a lot! Looks good overall, except I don't think this macro is worth it -- sorry.
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
e735f32
to
47caf16
Compare
@rustbot ready |
Ah, you force-pushed... please avoid that if at all possible (except when I ask you to do it), it makes it a lot harder to track what happened since the last review. (This is a pretty fatal flaw in Github, but unless they eventually get better at dealing with force-pushes we all have to work around it manually.) |
In this case you probably had to force-push to remove that macro commit, but you did not have to incorporate any of the recent changes in master. |
The existing concerns are resolved, but you added a bunch of new changes I didn't ask for so there's new concerns.^^ |
Ah shit, sorry. I'll remember that! |
Tests should be fine, the latest changes were just some refactorings. @rustbot ready |
This looks great, thanks! Please squash the commits using @rustbot author |
30c5a62
to
68b842c
Compare
@rustbot ready |
fixes #3967
The pr is structured in 4 commits, the first two replace the use of
Id
withRef
objects. The third commit refactors thedeclare_id!
macro into a macro that creates theseRef
objects for us. The last commit removes the usage ofSynchronisationObjects
.