hashmismatch / packed_struct.rs

Bit-level packing and unpacking for Rust
MIT License
164 stars 30 forks source link

Small signed integers don't round-trip (e.g. -1i8 -> 0b1111 -> 15i8) #63

Closed johnterickson closed 3 years ago

johnterickson commented 3 years ago

Hi- thanks for making this crate! One thing that was surprising to me is that small ints that are backed by signed types are not sign extended when they are unpacked. The end result is that these integers don't round trip.

If this is something that you'd accept as a pull request, I'd be happy to submit one with the right guidance. E.g. should sign extension happen on unpack or on Deref or does it need to be a new API?

Cheers, John

extern crate packed_struct;
#[macro_use]
extern crate packed_struct_codegen;

use packed_struct::prelude::*;

#[derive(PackedStruct)]
#[packed_struct(bit_numbering="lsb0",size_bytes="1")]
pub struct TestPack {
    #[packed_field(bits="0..=3")]
    tiny_signed_int: Integer<i8, packed_bits::Bits4>,
    #[packed_field(bits="4..=7")]
    tiny_unsigned_int: Integer<u8, packed_bits::Bits4>,

}
fn main() {
    let signed = -1i8;
    let unsigned = 1u8;
    let test = TestPack {
        tiny_signed_int: signed.into(),
        tiny_unsigned_int: unsigned.into(),
    };

    // pack into a byte array
    let packed: [u8; 1] = test.pack().unwrap();
    assert_eq!([0b00011111], packed);

    // unpack from a byte array
    let unpacked = TestPack::unpack(&packed).unwrap();
    assert_eq!(unsigned, *unpacked.tiny_unsigned_int);
    assert_eq!(signed, *unpacked.tiny_signed_int);
}

results in:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `-1`,
 right: `15`', src/main.rs:31:5
rudib commented 3 years ago

Thanks for reporting this issue! I'll look into it when I find some time.

The first feedback from me would be that I'd like to document this behavior a bit more. At least it should be specified that we are using one's complement to represent these integers. A welcome over the previous behavior, which was simply ignorant and broken :)

Also, I'll implement some min_value() and max_value() methods for these helpers as they will certainly be helpful.

Again, really thanks for reporting this, and I'll release 0.5.0 once this is ready to go.

rudib commented 3 years ago

0.5.0 is released with the fix, thanks for your contribution!

johnterickson commented 3 years ago

Awesome! Confirmed it works in my usage at https://github.com/johnterickson/lutcomp/commit/c29dd1ff85d0996970c172f9d9fefad5249c2ae8