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 new trait impls #141

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Add new trait impls #141

merged 1 commit into from
Nov 10, 2022

Conversation

joshlf
Copy link
Member

@joshlf joshlf commented Nov 10, 2022

Add the following impls:

  • str: Unaligned
  • NonZeroU8: Unaligned
  • NonZeroI8: Unaligned
  • Option<NonZeroU8>: Unaligned
  • Option<NonZeroI8>: Unaligned
  • MaybeUninit<T>: Unaligned where T: Unaligned

Previously, unsafe_impl! special-cased Unaligned and emitted a static assertion that the type in question actually had alignment 1. Since this used core::mem::align_of, which only works for Sized types, it was incompatible with adding an impl of Unaligned for str in this commit. In order to avoid this issue, this commit removes that special-casing, and introduces a new assert_unaligned! macro. Existing Unaligned implementations are modified to also call assert_unaligned!. assert_unaligned!(str) is not used since str: !Unaligned.

@joshlf
Copy link
Member Author

joshlf commented Nov 10, 2022

@djkoloski you may have thoughts here. I don't yet have safety comments for NonZeroX8: Unaligned or Option<NonZeroX8>: Unaligned, and my plan is to wait for rust-lang/rust#104082 to merge and then cite the updated documentation and then merge this PR at that point. However, if you can think of a different way to justify the soundness of those impls today (we can leave a TODO comment to cite the documentation once that PR lands), that'd be awesome.

@djkoloski
Copy link
Member

Maybe you could argue that since Option<NonZeroU8> and Option<NonZeroI8> are both guaranteed to be the same size as u8 and i8, that they must be the same size or smaller and so must have size 1. A size of 1 also implies an align of 1 since size must be a multiple of align.

@joshlf
Copy link
Member Author

joshlf commented Nov 10, 2022

Maybe you could argue that since Option<NonZeroU8> and Option<NonZeroI8> are both guaranteed to be the same size as u8 and i8, that they must be the same size or smaller and so must have size 1. A size of 1 also implies an align of 1 since size must be a multiple of align.

I was on the fence about taking that approach, but thinking about it more (and reinforced by you suggesting the same), I guess it's basically unthinkable that they would ever change that aspect of the layout of those types. I was thinking that the wording in that documentation is sufficiently weak (it says "is", not "is guaranteed to be" or something similar - it reads as though it could just be saying "in practice"), but I think that's probably not enough of a concern to block this. I'll go with that approach.

Add the following impls:
- `str: Unaligned`
- `NonZeroU8: Unaligned`
- `NonZeroI8: Unaligned`
- `Option<NonZeroU8>: Unaligned`
- `Option<NonZeroI8>: Unaligned`
- `MaybeUninit<T>: Unaligned` where `T: Unaligned`

Previously, `unsafe_impl!` special-cased `Unaligned` and emitted a
static assertion that the type in question actually had alignment 1.
Since this used `core::mem::align_of`, which only works for `Sized`
types, it was incompatible with adding an impl of `Unaligned` for `str`
in this commit. In order to avoid this issue, this commit removes that
special-casing, and introduces a new `assert_unaligned!` macro. Existing
`Unaligned` implementations are modified to also call
`assert_unaligned!`. `assert_unaligned!(str)` is not used since `str:
!Unaligned`.
@joshlf joshlf changed the title [WIP] Add new trait impls Add new trait impls Nov 10, 2022
@joshlf
Copy link
Member Author

joshlf commented Nov 10, 2022

OK done! Ready for review whenever you get a chance.

@joshlf joshlf merged commit 9dc10c8 into main Nov 10, 2022
@joshlf joshlf deleted the new-trait-impls branch November 10, 2022 23:11
joshlf added a commit that referenced this pull request Aug 3, 2023
Add the following impls:
- `str: Unaligned`
- `NonZeroU8: Unaligned`
- `NonZeroI8: Unaligned`
- `Option<NonZeroU8>: Unaligned`
- `Option<NonZeroI8>: Unaligned`
- `MaybeUninit<T>: Unaligned` where `T: Unaligned`

Previously, `unsafe_impl!` special-cased `Unaligned` and emitted a
static assertion that the type in question actually had alignment 1.
Since this used `core::mem::align_of`, which only works for `Sized`
types, it was incompatible with adding an impl of `Unaligned` for `str`
in this commit. In order to avoid this issue, this commit removes that
special-casing, and introduces a new `assert_unaligned!` macro. Existing
`Unaligned` implementations are modified to also call
`assert_unaligned!`. `assert_unaligned!(str)` is not used since `str:
!Unaligned`.
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