-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Wrap private padding fields with Padding newtype
#4609
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
base: main
Are you sure you want to change the base?
Conversation
src/types.rs
Outdated
| @@ -0,0 +1,12 @@ | |||
| use core::mem::MaybeUninit; | |||
|
|
|||
| /// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding. | |
| /// A transparent wrapper over `MaybeUninit<T>` to represent uninitialized padding | |
| /// while providing `Default`. |
To clarify why we don't just use Default
|
@rustbot blocked |
|
@mbyx could you put up a separate PR that only adds the |
b1e6991 to
e2a5397
Compare
Padding newtype
|
Would you mind rebasing this? CI will still fail because of the eq/ord traits, but with #4711 the |
e2a5397 to
271d31b
Compare
|
Reminder, once the PR becomes ready for a review, use |
77679b6 to
3f1c30f
Compare
|
Rebased. Seems to error because Also it seems that the style check fails at the first formatting error? My editor seems to have a different style for imports than |
That is what I was thinking, would you be able to add a commit that adds these implementations if
That config file changed somewhat recently, if you rebased without re-saving the files in your editor or running locally then it might not have picked that up. |
f7e75a4 to
33b5c86
Compare
|
That seems to fix most errors, except now we have a bit of an issue. One of the constants in For now I'll try to just not make that type using padding and see if I can get something mergeable. EDIT: Seems to be many more constants like this. Even if those are ignored for now a good number should still be wrapped. |
a66f7c2 to
6afc113
Compare
tgross35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the switches to Padding LGTM, just a few small things about the traits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you split the changes to this file to a separate commit? It's distinct from the rest of the mechanical changes in this PR.
| #[cfg(feature = "extra_traits")] | ||
| impl<T: Copy> Hash for Padding<T> { | ||
| fn hash<H: hash::Hasher>(&self, _state: &mut H) {} | ||
| } | ||
|
|
||
| #[cfg(feature = "extra_traits")] | ||
| impl<T: Copy> PartialEq for Padding<T> { | ||
| fn eq(&self, _other: &Self) -> bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are user-facing, so add a doc comment to each of these impls saying their behavior (i.e. that they give the results they do to act like the fields don't exist)
|
You should rebase as well, musl downloads have been flaky so I just switched us to a mirror |
6afc113 to
607e39d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
607e39d to
38d07d4
Compare
Description
Changes the types of fields with name
__pad,_pad,__padding,_padding,__rsvd,__unusedto use a new type wrapper aroundMaybeUninitas per #1453Only modifies private fields so that the changes can be backported.
Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI