-
Notifications
You must be signed in to change notification settings - Fork 105
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
Use copy_nonoverlapping
instead of slice::copy_from
#1448
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1448 +/- ##
=======================================
Coverage ? 87.45%
=======================================
Files ? 15
Lines ? 5166
Branches ? 0
=======================================
Hits ? 4518
Misses ? 648
Partials ? 0 ☔ View full report in Codecov by Sentry. |
src/lib.rs
Outdated
if bytes.len() == mem::size_of_val(self) { | ||
// SAFETY: Within this branch of the conditional, we have ensured | ||
// that `bytes.len()` (the size of the destination) is equal to | ||
// `mem::size_of_val(self)` the size of the source value. Neither | ||
// the size of the source nor the size of the destination change | ||
// between the above size check and the invocation of | ||
// `copy_unchecked`. The size of `self` (the source) cannot change, | ||
// because `Self` is bounded as `Immutable`, and the size of `bytes` | ||
// cannot change, because we hold an exclusive reference to it and | ||
// do not change its size. | ||
unsafe { util::copy_unchecked(self.as_bytes(), bytes) } |
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.
if bytes.len() == mem::size_of_val(self) { | |
// SAFETY: Within this branch of the conditional, we have ensured | |
// that `bytes.len()` (the size of the destination) is equal to | |
// `mem::size_of_val(self)` the size of the source value. Neither | |
// the size of the source nor the size of the destination change | |
// between the above size check and the invocation of | |
// `copy_unchecked`. The size of `self` (the source) cannot change, | |
// because `Self` is bounded as `Immutable`, and the size of `bytes` | |
// cannot change, because we hold an exclusive reference to it and | |
// do not change its size. | |
unsafe { util::copy_unchecked(self.as_bytes(), bytes) } | |
let self_bytes = self.as_bytes(); | |
if bytes.len() == self_bytes.len() { | |
// SAFETY: Within this branch of the conditional, we have ensured | |
// that `bytes.len()` (the size of the destination) is equal to | |
// `self_bytes.len()` (the size of the source). Neither | |
// the size of the source nor the size of the destination change | |
// between the above size check and the invocation of | |
// `copy_unchecked`. | |
unsafe { util::copy_unchecked(self_bytes, bytes) } |
(And reflow the safety commnet)
This makes the safety argument simpler since we don't need to reason about the relationship between size_of_val(self)
and self.as_bytes()
. It is probably ever so slightly tougher on the optimizer, but I doubt it's enough to make a difference.
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.
src/lib.rs
Outdated
// `Immutable`, and the size of `bytes` cannot change, because | ||
// we hold an exclusive reference to it and do not change its | ||
// size. | ||
unsafe { util::copy_unchecked(self.as_bytes(), bytes) } |
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.
Same here as above - do self.as_bytes()
first and bounds check using self_bytes.len()
.
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.
src/lib.rs
Outdated
// cannot change, because we hold an exclusive reference to it and do | ||
// not change its size. | ||
unsafe { | ||
util::copy_unchecked(self.as_bytes(), bytes); |
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.
Same here as above - do self.as_bytes()
first and bounds check using self_bytes.len()
.
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.
src/util.rs
Outdated
// SAFETY: This invocation satisfies the safety contract of | ||
// copy_nonoverlapping [1]: | ||
// - `src.as_ptr()` is trivially valid for reads of `src.len()` bytes | ||
// - `dst.as_ptr()` is valid for writes of `dst.len.len()` bytes, because 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.
// - `dst.as_ptr()` is valid for writes of `dst.len.len()` bytes, because the | |
// - `dst.as_ptr()` is valid for writes of `src.len()` bytes, because the |
Typo?
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.
6e1ee44
to
78d9f49
Compare
In doing so, we eliminate a potential panic path. Although I was not able to observe panic paths emitted in toy examples, they might be emitted in complex examples in which the optimizer is low on gas. Regardless, I expect this change will ease our future adoption of call-graph analysis techniques of potential panic paths. Ref #200 (comment)
78d9f49
to
bfea2dc
Compare
In doing so, we eliminate a potential panic path. Although I was not able to observe panic paths emitted in toy examples, they might be emitted in complex examples in which the optimizer is low on gas. Regardless, I expect this change will ease our future adoption of call-graph analysis techniques of potential panic paths.
Ref #200 (comment)