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

core-instrinctics, to core::mem::discriminant, but via the unsafe() mechanism #4

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

mcr
Copy link
Contributor

@mcr mcr commented Aug 20, 2024

obiwan-[projects/trentonio/minerva-voucher](2.6.6) mcr 10683 %cargo build
   Compiling minerva-voucher v0.8.10 (/ssw/projects/trentonio/minerva-voucher)
warning: the feature `core_intrinsics` is internal to the compiler or standard library
   --> src/lib.rs:175:12
    |
175 | #![feature(core_intrinsics)]
    |            ^^^^^^^^^^^^^^^
    |
    = note: using it is strongly discouraged
    = note: `#[warn(internal_features)]` on by default

while reading: https://doc.rust-lang.org/core/mem/fn.discriminant.html
I tried to replace like:

        core::intrinsics::discriminant_value(self)

with:

core::mem::discriminant(self)

but that returns a Discriminant, and I don't think that is helpful.
Reading further down, there is the unsafe() call, which this patch switches to.
Maybe that's a regression, and your opinion is sought.

@mcr mcr requested a review from j-devel August 20, 2024 20:51
@j-devel
Copy link
Collaborator

j-devel commented Aug 21, 2024

I tried to replace like: core::intrinsics::discriminant_value(self) with core::mem::discriminant(self) but that returns a Discriminant, and I don't think that is helpful.

I agree. (related - https://users.rust-lang.org/t/enum-crimes-getting-the-discriminant/88103/10)

Reading further down, there is the unsafe() call, which this patch switches to.

Since each enum here already "opted-in to having a primitive representation" (#[repr(u64)] for enum Sid, #[repr(u8)] for enum Yang, #[repr(u8)] for enum Attr), I think we can just use the accessing technique described in that section like your patch does.

@j-devel j-devel merged commit 3735d86 into master Aug 21, 2024
1 check passed
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