-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add safety sections, fix clippy warnings #73
Conversation
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.
lgtm except for one comment
src/arc_borrow.rs
Outdated
@@ -46,6 +46,9 @@ impl<'a, T> ArcBorrow<'a, T> { | |||
/// e.g. if we obtain such a reference over FFI | |||
/// TODO: should from_ref be relaxed to unsized types? It can't be | |||
/// converted back to an Arc right now for unsized types. | |||
/// | |||
/// # Safety | |||
/// - The reference to `T` must be valid |
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.
must be a reference to a T that came from an Arc.
"Reference must be valid" is not meaningful, it is already UB to have an invalid reference
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.
Ah, true! Do you think this function needs a safety section then?
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.
Or, if calling it with an invalid reference is already UB, and we are not doing anything that can cause UB, does this function need to be unsafe?
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.
Yes, this function needs a safety section, because calling it with an arbitrary &T can cause unsoundness down the line. It must be called with an &T that came from an Arc
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, thank you. Is like a from_raw
?
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.
Yes. In this case it is that this reference must have been obtained from a triomphe Arc, UniqueArc, or ArcBorrow.
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.
Great, makes sense!
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.
why remove it?
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.
Is this better:
# Safety
- The reference to `T` must have come from a Triomphe Arc, UniqueArc, or ArcBorrow.
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.
yes
As described above, I added safety sections to the docs and fixed the clippy warnings.