Manishearth commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1803313498
##########
arrow-buffer/src/util/bit_mask.rs:
##########
@@ -127,16 +127,16 @@ unsafe fn set_upto_64bits(
}
/// # Safety
-/// The caller must ensure all arguments are within the valid range.
+/// The caller must ensure `data` has `offset..(offset + 8)` range, and `count
<= 8`.
#[inline]
unsafe fn read_bytes_to_u64(data: &[u8], offset: usize, count: usize) -> u64 {
debug_assert!(count <= 8);
- let mut tmp = std::mem::MaybeUninit::<u64>::new(0);
+ let mut tmp: u64 = 0;
Review Comment:
> I initially did that but Miri did not allow to leave uninitialized bytes
even those are unused
Yep, like I said, it's UB in this case.
> So I quickly switched to new(0) and checked the performance implications
of the change. Perhaps I should have experimented 0 at the time.
yes, the use of MaybeUninit here at best would make performance _worse_ if
things don't get inlined (though they ought to). There's nothing special about
MaybeUninit that helps it make things better here. MaybeUninit is only special
in that it allows you to create uninitialized states without it being instantly
UB, but that's not what's happening here.
I would recommend trying to avoid unsafe used in this way, it's not a good
sign if a crate uses a lot of completely avoidable unsafe, especially when the
unsafe code displays a misunderstanding of the underlying abstractions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
