Draft backward incompatible: proposal to address UB FixedInt
#30
Conversation
|
Thank you, your changes clean up a bunch of old work-arounds back from (I checked blame) around Rust 1.13. It's definitely nicer like this. |
|
Hey - are you interested in getting this merged? I have one or another idea on how to improve this further, but I really like your proposed structure. |
|
|
||
| /// `FixedInt` provides encoding/decoding to and from fixed int representations. | ||
| /// | ||
| /// The emitted bytestring contains the bytes of the integer in machine endianness. | ||
| pub trait FixedInt: Sized + Copy { | ||
| const REQUIRED_SPACE: usize; | ||
| /// Returns how many bytes are required to represent the given type. | ||
| fn required_space() -> usize; |
There was a problem hiding this comment.
Why do you remove the required_space function/constant? I agree that it is slightly cludgey, but the intent is that it gives easy access to the size of an encoded integer when using the encode_fixed() and decode_fixed() methods (e.g. when incrementally parsing a packet). Or will we tell users to simply use mem::size_of?
There was a problem hiding this comment.
nevermind; mem::size_of probably takes care of this best.
|
|
||
| dst.clone_from_slice(encoded); | ||
| assert_eq!(dst.len(), size_of::<Self>()); | ||
| dst.clone_from_slice(&self.to_le_bytes()); | ||
| } | ||
|
|
||
| #[cfg(target_endian = "little")] |
There was a problem hiding this comment.
I believe it was a mistake to introduce the target-dependent methods here. The byte order of some encoded data doesn't need to match the target endianness after all. (This is a note to myself, nothing you need to fix here.)
FixedInt::REQUIRED_SPACEFixedInt::Bytesfn required_spaceswitch_endiannessimplementation requiredforbid(unsafe_code)