Use repr(u8) on State/Action

[thomcc]

Right now they'd be repr(Rust), which isn't guaranteed. If that repr changed (for example under a hypothetical -Z repr-rust-layout=randomize, which has been discussed), this code would fail to compile (the transmute later requires u8).

[thomcc]

Build failures are unrelated clippy errors due to new clippy.

[chrisduerr]

There's no reason to make these changes.

[thomcc]

It's a trivial change that keeps you from relying on not-guaranteed properties...

[TheEdward162]

@chrisduerr This is clearly UB as it doesn't have a stable representation (source) and then is used here, which is C level evilness, except the layout isn't even stable.

Furthermore, this code clearly does something highly unsafe, prone to memory invariant volation, and instead of marking the function unsafe it just says "Bad things will happen if those invariants are violated" in the doc comment. Go back to the C corner and be ashamed.

[TheEdward162]

To offer a solution with my shaming, how about reimplementing the unpack function in the following manner:

enum Foo {
    A = 0,
    B = 1,
    C = 2,
    D = 3,
    E = 4,
    F = 5,
    G = 6,
    H = 7,
    I = 8,
    J = 9,
    K = 10,
    L = 11,
    M = 12,
    N = 13,
    O = 14,
    P = 15
}

fn unpack(value: u8) -> Foo {
    match value & 0x0F {
        0 => Foo::A,
        1 => Foo::B,
        2 => Foo::C,
        3 => Foo::D,
        4 => Foo::E,
        5 => Foo::F,
        6 => Foo::G,
        7 => Foo::H,
        8 => Foo::I,
        9 => Foo::J,
        10 => Foo::K,
        11 => Foo::L,
        12 => Foo::M,
        13 => Foo::N,
        14 => Foo::O,
        15 => Foo::P,
        _ => unsafe { std::hint::unreachable_unchecked() }
    }
}

I know it's very verbose, but here the scope of unsafe doesn't reach beyond the function (i.e. to an enum definition outside the function) and is easily provable, so in my eyes it's way better. It also doesn't yell about dead code (or does and thus points at a possible error in implementation) and doesn't even require a stable repr on the enum, although since pack uses the numbers again the explicit values would stay.

I'm not sure about the performance implications, it's only my guess that the compiler might be able to see the pattern and hopefully do the transmute anyway, but if absolute performance isn't a requirement the unsafe can even be replaced by unreachable!() macro to just panic, or a panic in debug mode and unreachable_unchecked in release mode.

The verbosity could also be lightened with a macro if that's an issue, but I personally don't think it's needed, the file is short anyway.