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

Zeroable should be implemented for Atomic* #74

Closed
eira-fransham opened this issue Aug 24, 2021 · 4 comments · Fixed by #157
Closed

Zeroable should be implemented for Atomic* #74

eira-fransham opened this issue Aug 24, 2021 · 4 comments · Fixed by #157

Comments

@eira-fransham
Copy link

eira-fransham commented Aug 24, 2021

The atomic types are all bytemuck-compatible, and in fact as far as I know all bytemuck traits can be implemented for Atomic* that can be implemented for the underlying integer type as the operations available on the atomic types are a strict superset (i.e. setting their value via an immutable reference). Even casting between a non-atomic int and an atomic int should be safe, although from this discussion here rust-lang/rust#76314 it may be invalid to cast an &usize (or other scalar) to an &AtomicUsize (or other atomic scalar). This sadly means that Pod cannot be implemented without adding new traits to account for types where owned types can be cast but references cannot.

@Lokathor
Copy link
Owner

If it's unsafe to cast &usize to &AtomicUsize then the AtomicUsize type doesn't fit with Pod, because Pod allows casting between &T and &U when the size and alignment match.

Similarly, the TransparentWrapper trait isn't appropriate.

It's fine to have Zeroable, I suppose.

@eira-fransham
Copy link
Author

eira-fransham commented Aug 25, 2021

I think it might make sense to have new traits for immutable access, since that would allow safe casts to/from Cell/UnsafeCell/Atomic*, but even just Zeroable would be enough since that would allow the parking_lot mutexes to be zeroable and therefore it would be possible to safely create a heap-allocated array of them using calloc using the helpers in this crate.

I'll put a PR in to implement Zeroable and leave splitting the traits to later, since it's not necessary right now.

@Lokathor
Copy link
Owner

Lokathor commented Aug 26, 2021

I don't think you should read atomic values non-atomically.

@eira-fransham eira-fransham changed the title Most bytemuck traits should be implemented for Atomic* Zeroable should be implemented for Atomic* Aug 26, 2021
@eira-fransham
Copy link
Author

Yeah, very good point. Well, at least zeroable should be implemented.

nolanderc added a commit to nolanderc/bytemuck that referenced this issue Dec 19, 2022
Lokathor pushed a commit that referenced this issue Dec 21, 2022
* `impl Zeroable for Atomic*`

fixes #74

* added feature flag for `zeroable_atomics`

Added documentation for some feature flags on the `Zeroable` trait.
Also fixed some invalid references in doc-comments.
leod pushed a commit to leod/bytemuck that referenced this issue Jun 3, 2023
* `impl Zeroable for Atomic*`

fixes Lokathor#74

* added feature flag for `zeroable_atomics`

Added documentation for some feature flags on the `Zeroable` trait.
Also fixed some invalid references in doc-comments.
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 a pull request may close this issue.

2 participants