Skip to content
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 MaybeUninit properly #1572

Open
YaLTeR opened this issue Aug 21, 2019 · 4 comments
Open

Use MaybeUninit properly #1572

YaLTeR opened this issue Aug 21, 2019 · 4 comments

Comments

@YaLTeR
Copy link
Collaborator

YaLTeR commented Aug 21, 2019

Now that we switched to MaybeUninit in #1292 we might as well start using it properly; right now as far as I can tell it's equivalent to mem::uninitialized() in our usage. See the MaybeUninit docs.

In particular, instead of doing MaybeUninit::uninit().assume_init() we should be doing MaybeUninit::uninit(), then initializing the value using .as_mut_ptr().write(...) and only then doing .assume_init().

@shssoichiro
Copy link
Collaborator

It appears the only remaining place where this needs to be fixed is in AlignedArray::uninitialized.

@kornelski
Copy link
Contributor

How to handle PlaneRegionMut? It's used in lots of places for dst: &mut PlaneRegionMut<T>.

It's relatively easy to remove T: Pixel bound on it, and make PlaneRegionMut<MaybeUninit<T>> work.

Alternatively, maybe there could be a simpler type that is purely {width, height, stride} that could be created ad-hoc from PlaneRegionMut and slices? It seems that many places that take PlaneRegionMut don't actually use all of the plane config complexity, and just redundantly compute things like let xsize = (8 >> xdec) as isize; to reduce it to back basic width/height/stride.

@lu-zero
Copy link
Collaborator

lu-zero commented Oct 20, 2023

When do we have a PlaneRegion that is pointing to something not initialized though?

@kornelski
Copy link
Contributor

subpel_diamond_search makes PlaneRegionMut from let mut buf: Aligned<[T; 128 * 128]> = unsafe { Aligned::uninitialized() } and then hopes get_subpel_mv_rd is initializing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants