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

Add read/write implems for generic arrays #235

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Add read/write implems for generic arrays #235

merged 1 commit into from
Oct 5, 2021

Conversation

xlambein
Copy link
Contributor

Hey! Thanks for the great project :-)

I needed to (de)serialize arrays of structs that implement DekuRead/Write, so I went ahead and added those trait implementations. They also supersede the implementations for all the uX/iX/fX array/slices, so I removed those.

There's a bit of unsafe code to avoid doing unnecessary allocations.

#[allow(clippy::uninit_assumed_init)]
// This is safe because we initialize the array immediately after,
// and never return it in case of error
let mut slice: [T; N] = unsafe { MaybeUninit::uninit().assume_init() };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is because of potential performance impacts of default'ing here? let mut slice: [$typ; N] = [<$typ>::default(); N];

Copy link
Contributor Author

@xlambein xlambein Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using default() requires that T implements Default, which imo is an unnecessary restriction. To me the safe alternative would be to do:

let mut v = Vec::with_capacity(N);
let mut rest = input;
for i = 0..N {
    let (new_rest, value) = T::read(rest, ctx)?;
    v.push(value);
    rest = new_rest;
}
Ok((rest, v.try_into().unwrap()))

That makes a few unnecessary allocations and checks, which are avoided with the unsafe section. It doesn't seem very critical, so if you'd rather do that I think it's fine! :-)

@sharksforarms
Copy link
Owner

Hey @xlambein sorry this took so long, this PR looks good to me.

@sharksforarms sharksforarms merged commit 34f8f6b into sharksforarms:master Oct 5, 2021
@sharksforarms
Copy link
Owner

Released in 0.12.4, thanks @xlambein and sorry for the delay

@xlambein
Copy link
Contributor Author

Oh, I missed the previous comment. Thanks a lot for merging! And no worries about the delay, thanks for maintaining this project!

@sharksforarms
Copy link
Owner

i.r. to the use of unsafe, we could utilize this: rust-lang/rust#75644 once it's released!

@wcampbell0x2a
Copy link
Collaborator

Tracking issue: rust-lang/rust#89379

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

Successfully merging this pull request may close these issues.

3 participants