jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
545 stars 35 forks source link

ReadEndian implementation ignored when calling `read` #223

Closed tmwsnrx closed 10 months ago

tmwsnrx commented 10 months ago

While calling read on a type that implements BinRead also requires an implementation of ReadEndian, the value of ENDIAN is effectively ignored and little endian is assumed.

#[inline]
fn read_args<R: Read + Seek>(reader: &mut R, args: Self::Args<'_>) -> BinResult<Self>
where
    Self: ReadEndian,
{
    Self::read_options(reader, Endian::Little, args)
}
csnover commented 10 months ago

Just to confirm, are you manually implementing BinRead and ReadEndian on a type? The ReadEndian trait was intended as marker type for a derived implementation, in which case the passed endian does not matter since it will be overridden. I don’t think there is any harm in changing this method to take the value from the trait, but I just want to make sure first that there isn’t some other bug.

csnover commented 10 months ago

Sorry, my previous comment is incorrect. It is not possible to take an endianness from this marker trait because it is not guaranteed to be a static endianness. The use case for this marker trait is to say what the implementation does, so if you use it, the BinRead implementation has to do what the marker trait says it does, not the other way around.

If there is some other issue or misunderstanding, please comment with some sample code and I’m glad to look into it for you. Thanks!

tmwsnrx commented 10 months ago

Now I see. I have a rather complex type with a binary representation that is very different from the way I want the struct to look like, so I wanted to write a custom parser function for this. Since this can only be done for fields and not for an entire struct I went with a manual implementation of BinRead. Using read_be on the type worked just fine, so I assumed using read together with implementing ReadEndianness and specifying big ending should yield the same result. This being a marker trait now makes much more sense to me as I just realized I can specify endianness in the parser itself function (the BinRead implementation) or the underlying type.

My bad, hanks for having a look into it anyway!

csnover commented 10 months ago

No worries, feedback like this is always appreciated since it is helpful to understand where things are confusing or create unnecessary friction. I have added a patch to the documentation to hopefully make it clearer for others how to handle these traits.

For your specific case, depending on your tolerance for conversion overhead, you may also consider deriving BinRead and writing an implementation From<BadDesign> for GoodDesign instead, which can sometimes be cleaner and easier to maintain than a manual implementation.