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

Improve consistency of derive(Byte*) documentation #2306

Open
wants to merge 1 commit into
base: v0.8.x
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 58 additions & 15 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2838,8 +2838,7 @@ unsafe fn try_read_from<S, T: TryFromBytes>(
Ok(unsafe { candidate.assume_init() })
}

/// Types for which a sequence of bytes all set to zero represents a valid
/// instance of the type.
/// Types for which a sequence of `0` bytes is a valid instance.
///
/// Any memory region of the appropriate length which is guaranteed to contain
/// only zero bytes can be viewed as any `FromZeros` type with no runtime
Expand Down Expand Up @@ -5443,14 +5442,36 @@ pub unsafe trait Unaligned {
Self: Sized;
}

/// Derives an optimized implementation of [`Hash`] for types that implement
/// [`IntoBytes`] and [`Immutable`].
/// Derives an optimized [`Hash`] implementation.
///
/// The standard library's derive for `Hash` generates a recursive descent
/// into the fields of the type it is applied to. Instead, the implementation
/// derived by this macro makes a single call to [`Hasher::write()`] for both
/// [`Hash::hash()`] and [`Hash::hash_slice()`], feeding the hasher the bytes
/// of the type or slice all at once.
/// This derive can be applied to structs and enums implementing both
/// [`Immutable`] and [`IntoBytes`]; e.g.:
///
/// ```
/// # use zerocopy_derive::{ByteHash, Immutable, IntoBytes};
/// #[derive(ByteHash, Immutable, IntoBytes)]
/// #[repr(C)]
/// struct MyStruct {
/// # /*
/// ...
/// # */
/// }
///
/// #[derive(ByteHash, Immutable, IntoBytes)]
/// #[repr(u8)]
/// enum MyEnum {
/// # Variant,
/// # /*
/// ...
/// # */
/// }
/// ```
///
/// The standard library's [`derive(Hash)`][derive@Hash] produces hashes by
/// individually hashing each field and combining the results. Instead, the
/// implementations of [`Hash::hash()`] and [`Hash::hash_slice()`] generated by
/// `derive(ByteHash)` convert the entirey of `self` to a byte slice and hashes
/// it in a single call to [`Hasher::write()`].
Comment on lines +5470 to +5474
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention something about how this is often faster or easier to optimize or something to that effect? After all, that's the only reason you'd use this derive. Currently the only mention of optimization or performance in this doc comment is in the summary sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "only mention" being in the summary sentence is fairly prominent positioning. I'm hesitant to over-promise here, because this derive isn't necessarily a performance slam dunk. For example, comparing two u64s for equality is likely to optimize better than casting them to slices of dynamic length and comparing them byte-by-byte. I'm really not sure where the inflection point is.

For that reason, I think the honest thing to do is to explain the mechanics, but not make specific promises about performance.

///
/// [`Hash`]: core::hash::Hash
/// [`Hash::hash()`]: core::hash::Hash::hash()
Expand All @@ -5459,13 +5480,35 @@ pub unsafe trait Unaligned {
#[cfg_attr(doc_cfg, doc(cfg(feature = "derive")))]
pub use zerocopy_derive::ByteHash;

/// Derives an optimized implementation of [`PartialEq`] and [`Eq`] for types
/// that implement [`IntoBytes`] and [`Immutable`].
/// Derives optimized [`PartialEq`] and [`Eq`] implementations.
///
/// This derive can be applied to structs and enums implementing both
/// [`Immutable`] and [`IntoBytes`]; e.g.:
///
/// ```
/// # use zerocopy_derive::{ByteEq, Immutable, IntoBytes};
/// #[derive(ByteEq, Immutable, IntoBytes)]
/// #[repr(C)]
/// struct MyStruct {
/// # /*
/// ...
/// # */
/// }
///
/// #[derive(ByteEq, Immutable, IntoBytes)]
/// #[repr(u8)]
/// enum MyEnum {
/// # Variant,
/// # /*
/// ...
/// # */
/// }
/// ```
///
/// The standard library's derive for [`PartialEq`] generates a recursive
/// descent into the fields of the type it is applied to. Instead, the
/// implementation derived by this macro performs a single slice comparison of
/// the bytes of the two values being compared.
/// The standard library's [`derive(Eq, PartialEq)`][derive@PartialEq] computes
/// equality by individually comparing each field. Instead, the implementation
/// of [`PartialEq::eq`] emitted by `derive(ByteHash)` converts the entirey of
/// `self` and `other` to byte slices and compares those slices for equality.
Comment on lines +5508 to +5511
Copy link
Member

Choose a reason for hiding this comment

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

Same here re: mentioning performance

#[cfg(any(feature = "derive", test))]
#[cfg_attr(doc_cfg, doc(cfg(feature = "derive")))]
pub use zerocopy_derive::ByteEq;
Expand Down
Loading