The scope of the unsafe block can be appropriately reduced #4700
Conversation
✅ Deploy Preview for tokio-rs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| self.num_initialized = parts.initialized; | ||
| unsafe { | ||
| self.num_initialized = parts.initialized; | ||
| vec.set_len(parts.len); | ||
| } |
There was a problem hiding this comment.
I'm not so sure that this makes the code easier to read. The assignment to num_initialized is important for the safety of the set_len call too.
There was a problem hiding this comment.
Although this assignment is important for its safety, I think the unsafe block should try to include only those places that are really unsafe, so that you can clearly focus on certain parameters of the unsafe function.
tokio/src/process/windows.rs
Outdated
| impl Drop for Waiting { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| let rc = UnregisterWaitEx(self.wait_object, INVALID_HANDLE_VALUE); | ||
|
|
||
| let rc = unsafe { UnregisterWaitEx(self.wait_object, INVALID_HANDLE_VALUE) }; | ||
| if rc == 0 { | ||
| panic!("failed to unregister: {}", io::Error::last_os_error()); | ||
| } | ||
| drop(Box::from_raw(self.tx)); | ||
| } | ||
| drop(unsafe { Box::from_raw(self.tx) }); | ||
|
|
||
| } | ||
| } |
|
There are some more things that need rustfmt. You can fix them with the following command: |
Thank you for your suggestion |
|
You will need to merge in master to fix the CI failures. |
|
I'm not really sure why CI failed here. |
I believe it's cross-rs/cross#724, removing the cache should fix it. |
|
Uhh, what. I tried merging in master to fix the CI failures, but it closed the PR instead, and it wont let me reopen it. @peamaeq Can you please reopen this PR, of if unable, open a new one? You will need the newest changes from Tokio master for CI to pass. |
Motivation
Solution