runtime: improve safety comments of Readiness<'_>
#7415
Conversation
tokio/src/runtime/io/scheduled_io.rs
Outdated
| let (scheduled_io, state, waiter) = { | ||
| // Safety: `Self` is `!Unpin` | ||
| let me = unsafe { self.get_unchecked_mut() }; | ||
| (me.scheduled_io, &mut me.state, &me.waiter) |
There was a problem hiding this comment.
For reviewers: The reason I didn't use pin_project to eliminate the unsafe block is that Future's implementation already has a lot of unsafe blocks, so eliminating it doesn't significantly reduce the mental burden but increases code complexity.
There was a problem hiding this comment.
Take it or leave it: perhaps that's worth including in the comment?
tokio/src/runtime/io/scheduled_io.rs
Outdated
| // Safety: We don't need to acquire the lock here because | ||
| // the `waiter.interest` never changes. | ||
| let interest = unsafe { (*waiter.get()).interest }; |
There was a problem hiding this comment.
Could it be shared in this scenario? You removed the comment that says it can't - is that because it actually can or just because you don't need it?
There was a problem hiding this comment.
Could it be shared in this scenario?
The State::Done certainly indicates waiter is not shared, the original comment is right.
is that because it actually can or just because you don't need it?
Because the original comment is not comprehensive.
waiter.interest is not shared in this scenario, but this doesn't mean we can always observe the latest value of waiter.interest without the lock held, unless waiter.interest is never changes.
There was a problem hiding this comment.
I have enriched the safety comment for your concern: af0a52a .
tokio/src/runtime/io/scheduled_io.rs
Outdated
| let (scheduled_io, state, waiter) = { | ||
| // Safety: `Self` is `!Unpin` | ||
| let me = unsafe { self.get_unchecked_mut() }; | ||
| (me.scheduled_io, &mut me.state, &me.waiter) |
There was a problem hiding this comment.
Take it or leave it: perhaps that's worth including in the comment?
ADD-SP
force-pushed
the
add_sp/runtime-write-more-detailed-safety-comment
branch
2 times, most recently
from
029df9f to
b8c9124
Compare
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
ADD-SP
force-pushed
the
add_sp/runtime-write-more-detailed-safety-comment
branch
from
b8c9124 to
6e6b73e
Compare
| // Safety: Since the `waiter` is not in the intrusive list yet, | ||
| // so we have exclusive access to it. The Mutex ensures |
There was a problem hiding this comment.
Sentences starting with "since" don't use "so".
| // Safety: Since the `waiter` is not in the intrusive list yet, | |
| // so we have exclusive access to it. The Mutex ensures | |
| // Safety: Since the `waiter` is not in the intrusive list yet, | |
| // we have exclusive access to it. The Mutex ensures |
Motivation
Just some improvements while reading the source.
Solution
Nothing to point out in particular.