Skip to content

Conversation

@nicoburns
Copy link
Contributor

Motivation is exactly the same as the from_u32 method: it allows interop with systems that represent tag in this format (HarfBuzz, CoreText, etc).

Signed-off-by: Nico Burns <nico@nicoburns.com>
Copy link
Member

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

LGTM with a couple changes requested.

Self::from_be_bytes(src.to_be_bytes())
}

/// Convert a `Tag` into a big-endian `u32`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Convert a `Tag` into a big-endian `u32`
/// Convert a `Tag` into a big-endian `u32`.

///
/// This is provided as a convenience method for interop with code that
/// stores tags as big-endian u32s.
pub const fn into_u32(self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

I know a conversion consuming self should have an into_ prefix but the vast majority of the methods in this crate use to_ so I'd prefer to_u32 here for consistency.

@nicoburns nicoburns marked this pull request as draft September 12, 2025 11:09
@nicoburns
Copy link
Contributor Author

Holding off on this until I can determine whether there a consistent byte order used to represent opentype tags as u32. It is looking possible that macOS and Windows both use u32 but with swapped byte order.

@behdad
Copy link
Contributor

behdad commented Sep 12, 2025

Holding off on this until I can determine whether there a consistent byte order used to represent opentype tags as u32. It is looking possible that macOS and Windows both use u32 but with swapped byte order.

Please add one that reads it as big-endian, like the rest of the most of the font. I think you are right that Windows uses the opposite order.

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.

3 participants