Skip to content

Conversation

Aditya-PS-05
Copy link
Contributor

closes #1778

Copy link

google-cla bot commented Oct 30, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

src/lib.rs Outdated
Comment on lines 1194 to 1195
/// # use zerocopy::{TryFromBytes, KnownLayout, IntoBytes, Immutable};
/// #[derive(Debug, KnownLayout, Immutable, IntoBytes, TryFromBytes)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// # use zerocopy::{TryFromBytes, KnownLayout, IntoBytes, Immutable};
/// #[derive(Debug, KnownLayout, Immutable, IntoBytes, TryFromBytes)]
/// # use zerocopy_derive::TryFromBytes;
/// #[derive(TryFromBytes)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I thought it relates to example. sorry for that. I am going to change it.

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.46%. Comparing base (a80c2d4) to head (467071b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1993   +/-   ##
=======================================
  Coverage   89.46%   89.46%           
=======================================
  Files          16       16           
  Lines        5838     5838           
=======================================
  Hits         5223     5223           
  Misses        615      615           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

Just one last nit! Otherwise LGTM!

src/lib.rs Outdated
Comment on lines 1185 to 1204
/// # Portability
///
/// To ensure consistent endianness for enums with multi-byte representations,
/// explicitly specify and convert each discriminant:
///
/// Here, `DataStoreVersion` enforces little-endian encoding for each `u32`
/// discriminant, ensuring portability across platforms with differing native endianness.
///
/// ```
/// # use zerocopy_derive::TryFromBytes;
/// #[derive(TryFromBytes)]
/// #[repr(u32)]
/// pub enum DataStoreVersion {
/// /// Version 1 of the data store.
/// V1 = 9u32.to_le(),
///
/// /// Version 2 of the data store.
/// V2 = 10u32.to_le(),
/// }
/// ```
///
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tightening up the prose a bit:

Suggested change
/// # Portability
///
/// To ensure consistent endianness for enums with multi-byte representations,
/// explicitly specify and convert each discriminant:
///
/// Here, `DataStoreVersion` enforces little-endian encoding for each `u32`
/// discriminant, ensuring portability across platforms with differing native endianness.
///
/// ```
/// # use zerocopy_derive::TryFromBytes;
/// #[derive(TryFromBytes)]
/// #[repr(u32)]
/// pub enum DataStoreVersion {
/// /// Version 1 of the data store.
/// V1 = 9u32.to_le(),
///
/// /// Version 2 of the data store.
/// V2 = 10u32.to_le(),
/// }
/// ```
///
/// # Portability
///
/// To ensure consistent endianness for enums with multi-byte representations,
/// explicitly specify and convert each discriminant using `.to_le()` or
/// `.to_be()`; e.g.:
///
/// ```
/// # use zerocopy_derive::TryFromBytes;
/// // `DataStoreVersion` is encoded in little-endian.
/// #[derive(TryFromBytes)]
/// #[repr(u32)]
/// pub enum DataStoreVersion {
/// /// Version 1 of the data store.
/// V1 = 9u32.to_le(),
///
/// /// Version 2 of the data store.
/// V2 = 10u32.to_le(),
/// }
/// ```
///

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes done

@jswrenn jswrenn enabled auto-merge November 1, 2024 16:25
@jswrenn jswrenn added this pull request to the merge queue Nov 1, 2024
Merged via the queue into google:main with commit 6aba344 Nov 1, 2024
69 checks passed
google-pr-creation-bot pushed a commit to google-pr-creation-bot/zerocopy that referenced this pull request Feb 25, 2025
@joshlf
Copy link
Member

joshlf commented Feb 25, 2025

Backporting in #2390

github-merge-queue bot pushed a commit that referenced this pull request Feb 25, 2025
…2390)

Co-authored-by: Aditya Pratap Singh <adityapratapsjnhh7654@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how to implement enums with endian-specific tags
4 participants