Skip to content

Conversation

@joshlf
Copy link
Member

@joshlf joshlf commented Jan 31, 2026


Latest Update: v3 β€” Compare vs v2

πŸ“š Full Patch History

Links show the diff between the row version and the column version.

Version v2 v1 Base
v3 vs v2 vs v1 vs Base
v2 vs v1 vs Base
v1 vs Base

Stacked PRs enabled by GHerrit.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the zerocopy crate by introducing a new Atomic<T> type. This type allows for safe and efficient atomic operations on user-defined data structures that are IntoBytes and fit within the size constraints of native atomic primitives (up to 8 bytes). By leveraging Rust's core::sync::atomic module, it provides new, load, store, compare_exchange, and fetch_update functionalities, making it easier to work with concurrent data without manual unsafe block management for atomic conversions.

Highlights

  • New Atomic<T> Type: Introduced a new Atomic<T> struct that enables atomic operations on custom types up to 8 bytes in size, leveraging Rust's core::sync::atomic primitives.
  • Type-to-Atomic Mapping: Implemented Kl and AtomicSelector traits along with helper functions (into_atomic, from_atomic) to dynamically select the appropriate underlying atomic primitive (AtomicU8, AtomicU16, AtomicU32, AtomicU64) based on the size of the generic type T.
  • Simplified Integration: Provided an impl_atomic! macro to easily implement the Kl trait for various primitive types (e.g., u8, u16, u32, u64) and byte arrays, simplifying the use of Atomic<T>.
  • Dependency Updates: Updated the zerocopy and zerocopy-derive dependencies in Cargo.lock to version 0.8.37.
  • Minor Typo Fix: Corrected a small typo in a comment within the src/util/macros.rs file.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with πŸ‘ and πŸ‘Ž on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@joshlf
Copy link
Member Author

joshlf commented Jan 31, 2026

This is an alternate design to #2956, this one anchored on KnownLayout.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a generic Atomic<T> wrapper, which is a useful abstraction for atomic operations on various types. The overall approach using a Kl trait to select the appropriate underlying standard atomic type is clever.

However, as an initial commit, there are several areas that need attention before this can be considered complete and safe. The most critical issue is the pervasive use of // SAFETY: TODO comments throughout the new src/atomic.rs file. Given the heavy use of unsafe code, providing detailed safety justifications is paramount for ensuring the soundness of this abstraction.

Additionally, there are several TODO comments for documentation and missing trait implementations (like Debug, Copy, Clone) on Atomic<T> that should be addressed. Some of the tests also appear to be using methods (swap, compare_exchange, etc.) that are not yet implemented on Atomic<T>, causing them to fail compilation.

I've left specific comments on these points.

Comment on lines +67 to +83
// SAFETY: TODO
unsafe { u.prim }
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This unsafe block lacks a safety justification. Reading from a union field that was not the last one written to is undefined behavior unless the types are layout-compatible. Please provide a detailed safety comment explaining why it's safe to transmute t into <T::Atomic as AtomicOps>::Value. This likely relies on T being IntoBytes (which implies no padding) and having a size that fits within the primitive type.

Comment on lines +81 to +97
// SAFETY: TODO
ManuallyDrop::into_inner(unsafe { u.t })
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This unsafe block lacks a safety justification. Please add a safety comment explaining why it's safe to transmute from the primitive value v to a MaybeUninit<T>. This is a critical part of the load operation's soundness.

src/atomic.rs Outdated
Comment on lines 215 to 216
// SAFETY: TODO
unsafe { mu.assume_init() }
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This is a critical unsafe block that lacks a safety justification. The call to mu.assume_init() is only sound if the bytes read from the atomic operation correspond to a valid instance of T.

The current implementation of load has a bound of T: Kl, but Kl does not require T: FromBytes. This means a user could implement Kl for a type with validity invariants (like bool), and calling load would be unsound.

To ensure soundness, you should either:

  1. Add a T: FromBytes bound to load.
  2. Make Kl an unsafe trait that requires implementors to guarantee that T is FromBytes.
  3. If you intend to support types that are not FromBytes, load would need to perform validation (e.g., require T: TryFromBytes and check is_bit_valid).

Please add a detailed safety comment explaining how the invariants of Atomic<T> guarantee the soundness of this operation.

src/atomic.rs Outdated
Comment on lines 89 to 101
/// # Safety
///
/// TODO
#[doc(hidden)]
pub unsafe trait AtomicSelector<const N: usize> {
type AtomicType;
const PADDING: usize;
}

/// # Safety
///
/// TODO
#[doc(hidden)]
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The safety documentation for the unsafe traits AtomicSelector and AtomicOps is missing. It's crucial to document the invariants that implementors must uphold to ensure safe usage.

src/atomic.rs Outdated
Comment on lines 123 to 153
macro_rules! impl_atomic_selector {
($atomic:ty [$value:ty]; $($size:expr),+) => {
// SAFETY: TODO
unsafe impl AtomicOps for $atomic {
type Value = $value;

#[inline(always)]
fn new(value: Self::Value) -> Self {
Self::new(value)
}

#[inline(always)]
fn load(&self, ordering: Ordering) -> Self::Value {
self.load(ordering)
}

#[inline(always)]
fn store(&self, value: Self::Value, ordering: Ordering) {
self.store(value, ordering)
}
}

$(
// SAFETY: TODO
unsafe impl AtomicSelector<$size> for () {
type AtomicType = $atomic;
const PADDING: usize = core::mem::size_of::<$atomic>() - $size;
}
)+
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The unsafe impl blocks within the impl_atomic_selector! macro are missing safety justifications. Please document why these implementations of AtomicOps and AtomicSelector are safe.

Comment on lines +239 to +243
fn test_atomic_u32() {
let a = Atomic::new(10u32);
assert_eq!(a.load(Ordering::SeqCst), 10);
a.store(20, Ordering::SeqCst);
assert_eq!(a.load(Ordering::SeqCst), 20);
assert_eq!(a.swap(30, Ordering::SeqCst), 20);
assert_eq!(a.load(Ordering::SeqCst), 30);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This test calls a.swap(...), but the swap method is not implemented on Atomic<T>, causing the test to fail compilation. The same issue exists for compare_exchange and fetch_update in other tests in this file. These methods should be implemented on Atomic<T> for the tests to pass.

src/atomic.rs Outdated
ManuallyDrop::into_inner(unsafe { u.t })
}

trait Kl {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The trait name Kl is not very descriptive. Consider renaming it to something that better reflects its purpose, such as AtomicLayout or KnownAtomicLayout, to improve code readability.

src/atomic.rs Outdated
Comment on lines 178 to 188
/// TODO
#[repr(transparent)]
#[allow(missing_copy_implementations, missing_debug_implementations)]
pub struct Atomic<T: Kl> {
// INVARIANT: The first `size_of::<T>()` bytes of `atomic` have the same
// validity as `T`, and logically own a `T`.
atomic: T::Atomic,
_marker: PhantomData<T>,
}

// TODO: Impl traits (Copy, Clone, Debug, Send, Sync, etc).
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The Atomic<T> struct and its methods are missing documentation. Additionally, important traits like Copy, Clone, Debug, Send, and Sync are not implemented yet, as noted by the TODO comment. Implementing these would make Atomic<T> more ergonomic and useful.

@joshlf joshlf force-pushed the Ge6a504780e973e00e15ad398613d1473ad57ae64 branch from dd6dc84 to 89ac12e Compare January 31, 2026 18:38
gherrit-pr-id: Ge6a504780e973e00e15ad398613d1473ad57ae64
@joshlf joshlf force-pushed the Ge6a504780e973e00e15ad398613d1473ad57ae64 branch from 89ac12e to f46f756 Compare January 31, 2026 18:41
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.

1 participant