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

[derive] Support derive(TryFromBytes) for structs #665

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Dec 1, 2023

Supersedes #370.

Makes progress on #5.

@jswrenn jswrenn force-pushed the derive-tryfrombytes-struct branch 8 times, most recently from bface06 to a1b7324 Compare December 7, 2023 17:48
@jswrenn jswrenn marked this pull request as ready for review December 7, 2023 17:51
@jswrenn jswrenn requested a review from joshlf December 7, 2023 17:52
Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

We also need tests which actually execute the derived is_bit_valid implementation. Our existing TryFromBytes tests in zerocopy only exercise it for hand-rolled implementations.

src/util.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/ext.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the derive-tryfrombytes-struct branch 2 times, most recently from b82bd97 to e1e2a13 Compare December 7, 2023 20:48
// - The size of the object referenced by the resulting pointer is equal to
// the size of the object referenced by `self`.
// - The alignment of `Unsized` is equal to the alignment of `[u8]`.
let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Two) };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joshlf Is there something I should be doing here instead of making cast_unsized public?

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be to create an array, initialize it to valid values, and then use try_from_ref. Make sure that returns true (which will validate that the array's size and alignment are correct) and then overwrite the bytes to make it invalid and call try_from_ref again and expect it to fail the second time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lol, I genuinely forgot try_from_ref existed. Actually, though, I think I prefer the current approach, since it allows us to focus the test on is_bit_valid (which is the item actually generated by derive(TryFromBytes)) and not take a dependency on KnownLayout.

Supersedes #370.

Makes progress on #5.

Co-authored-by: Joshua Liebow-Feeser <hello@joshlf.com>
@jswrenn jswrenn added this pull request to the merge queue Dec 8, 2023
Merged via the queue into main with commit e4b1f55 Dec 8, 2023
126 checks passed
@jswrenn jswrenn deleted the derive-tryfrombytes-struct branch December 8, 2023 16:15
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.

2 participants