Repository navigation
Unnecessary unsafe in anstyle-parse's osc_dispatch? #300
Description
Activity
I just ran across this
MaybeUnint::uninit.assume_init()which looks iffy (but is probably technically OK?) in a cargo-vet review. Glad to see I'm not alone in having some pause when reading this code 🙂It looks like this was changed from
mem::uninitialized()toMaybeUninit::uninit().assume_init()in this commit c093a67, but not sure why uninitialized was needed in the original addition of this function (3149ed0).I suppose it's in the hot-loop, where maybe even zeroing
[&[u8]; 16]has a measurable impact?At the very least, it seems like the creation of the
[MaybeUninit; MAX]could be safe.For the second unsafe block (casting to params) the recently stabilized slice function
assume_init_refin 1.93 would clarify the intent a bit better. I'm not sure on the MSRV policy for this crate/project though...I don't see any change in the emitted assembly for this small mockup https://godbolt.org/z/Ea51z9hKj
Illustrating both changes:
#[inline] fn osc_dispatch<P: Perform>(&self, performer: &mut P, byte: u8) { let mut slices: [MaybeUninit<&[u8]>; MAX_OSC_PARAMS] = - unsafe { MaybeUninit::uninit().assume_init() }; + [MaybeUninit::uninit(); MAX_OSC_PARAMS]; for (i, slice) in slices.iter_mut().enumerate().take(self.osc_num_params) { let indices = self.osc_params[i]; *slice = MaybeUninit::new(&self.osc_raw[indices.0..indices.1]); } + let params; unsafe { let num_params = self.osc_num_params; - let params = &slices[..num_params] as *const [MaybeUninit<&[u8]>] as *const [&[u8]]; + params = &slices[..num_params].assume_init_ref(); - performer.osc_dispatch(&*params, byte == 0x07); } + performer.osc_dispatch(params, byte == 0x07); }in a cargo-vet review.
That's also what I was doing when I saw this, I'm glad it's not just me wondering about this!
Illustrating both changes:
I think I'd initially thought of suggesting the same (except keeping the use of
asinstead ofassume_init_ref()since Cargo.toml has the MSRV being 1.66), but then decided to instead ask whyunsafewas used when it doesn't seem strictly necessary - I think it's the sort of thing that could do with a comment explaining why (e.g. if it does have a performance impact).This started as a fork of
vtparsebut I've not kept up with all of the latestvtparsechanges.
anstyle-parsehas:this caught my eye because I wasn't sure if the initialisation of
slicesis sound, but after some thought it's not clear whyMaybeUninitis needed at all - I don't really understand it, but is there any reason why you can't instead create an array of slices that is initialised using zero-length slices, and then overwrite them where possible? I.e.