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

extract raw traits for internal usage, #3 #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flier
Copy link

@flier flier commented Feb 19, 2024

No description provided.

Copy link
Member

@Phantomical Phantomical left a comment

Choose a reason for hiding this comment

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

I've gone and approved CI for this. Not sure why it needed approval - unless you haven't accepted the org invite yet? I'm also not sure if I'll have to approve again if you add more commits

I think you have the right idea here but I think that creating traits for these is not necessarily the right approach for this. I don't foresee anyone having a need to generically convert various raw xed types to their corresponding wrappers and there is a real cost to putting core methods in traits where they aren't immediately visible in the docs or, to a lesser extent, in autocomplete.

My suggestion is that we keep the wrapper type macro, but just change it to stamp out some inherent methods instead of having them be in traits. Something like this (you may need to reformat it to match):

/// Declare accessors converting between `$name` and `$raw`.
///
/// # Safety
/// `$name` must be a `#[repr(transparent)]` wrapper around `$raw`.
macro_rules! wrapper_type_accessors {
  ($name:type => $raw:type) => {
    impl $name {
      pub fn from_raw(raw: $raw) -> Self {
        Self(raw)
      }

      pub fn from_ref(raw: &$raw) -> Self {
        // SAFETY: The user of this macro asserts that $name is a #[repr(transparent)]
        //         wrapper around $raw
        unsafe { &*(raw as *const $raw as *const Self) }
      }

      pub fn into_raw(self) -> $raw {
        self.0
      }

      pub fn as_raw(&self) -> &$raw {
        &self.0
      }

      pub fn as_raw_mut(&mut self) -> &mut $raw {
        &mut self.0
      }
   }
}

and we could also create some doc comments for the various methods as opposed to them being empty.

If we do still need generic conversions on top of that they should probably be From impls for the relevant types.

#[doc = concat!(
"Create a `", stringify!($name), "` from the underlying enum value."
)]
pub const fn from_raw(value: u32) -> Option<Self> {
pub(crate) const fn from_raw(value: u32) -> Option<Self> {
Copy link
Member

Choose a reason for hiding this comment

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

That this is const is actually rather important. It means that users of xed can create their own constants for new variants introduced by xed-sys before we get around to adding them here.

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.

2 participants