Merge #521

521: [3/n] Remove u24 r=japaric a=Dirbaio Part 3 of N of #492 - Now that we're going to use rzCOBS for encoding the stream, extra zero bytes are not that expenseive. Using a u32 instead of a u24 adds one zero byte, which when encoded is just 1 extra bit. - Users are unlikely to use u24, as it's quite obscure (I didn't know it existed until I found it while reading defmt's source). - It makes the code more complicated, because it's not natively supported by Rust. In the code size optimizations of the macro codegen I'm working on, it really breaks the "symmetry" of the code. Therefore I propose we remove it. Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
knurling-rs · Jun 29, 2021 · 0a8d65b · 0a8d65b
2 parents fde2aab + d4c90e4
commit 0a8d65b
Show file tree

Hide file tree

Showing 6 changed files with 9 additions and 31 deletions.
diff --git a/book/src/ser-integers.md b/book/src/ser-integers.md
@@ -3,16 +3,16 @@
 Integers will be serialized in little endian order using `to_le_bytes()`.
 `usize` and `isize` values will be subject to LEB128 compression.
 
-``` rust
+```rust
 # extern crate defmt;
 defmt::error!("The answer is {=i16}!", 300);
 // on the wire: [3, 44, 1]
 //  string index ^  ^^^^^ `300.to_le_bytes()`
 //  ^ = intern("The answer is {=i16}!")
 
-defmt::error!("The answer is {=u24}!", 131000);
-// on the wire: [4, 184, 255, 1]
-//                  ^^^^^^^^^^^ 131000.to_le_bytes()[..3]
+defmt::error!("The answer is {=u32}!", 131000);
+// on the wire: [4, 184, 255, 1, 0]
+//                  ^^^^^^^^^^^^^^^ 131000.to_le_bytes()
 
 defmt::error!("The answer is {=usize}!", 131000);
 // on the wire: [4, 184, 255, 1]

diff --git a/decoder/src/decoder.rs b/decoder/src/decoder.rs
@@ -208,12 +208,6 @@ impl<'t, 'b> Decoder<'t, 'b> {
                 Type::Isize => args.push(Arg::Ixx(self.bytes.read_i32::<LE>()? as i128)),
                 Type::U8 => args.push(Arg::Uxx(self.bytes.read_u8()? as u128)),
                 Type::U16 => args.push(Arg::Uxx(self.bytes.read_u16::<LE>()? as u128)),
-                Type::U24 => {
-                    let data_low = self.bytes.read_u8()?;
-                    let data_high = self.bytes.read_u16::<LE>()?;
-                    let data = data_low as u128 | (data_high as u128) << 8;
-                    args.push(Arg::Uxx(data as u128));
-                }
                 Type::U32 => args.push(Arg::Uxx(self.bytes.read_u32::<LE>()? as u128)),
                 Type::U64 => args.push(Arg::Uxx(self.bytes.read_u64::<LE>()? as u128)),
                 Type::U128 => args.push(Arg::Uxx(self.bytes.read_u128::<LE>()? as u128)),
@@ -261,8 +255,7 @@ impl<'t, 'b> Decoder<'t, 'b> {
                     data = match size_after_truncation {
                         1 => self.bytes.read_u8()? as u128,
                         2 => self.bytes.read_u16::<LE>()? as u128,
-                        3 => self.bytes.read_u24::<LE>()? as u128,
-                        4 => self.bytes.read_u32::<LE>()? as u128,
+                        3..=4 => self.bytes.read_u32::<LE>()? as u128,
                         5..=8 => self.bytes.read_u64::<LE>()? as u128,
                         9..=16 => self.bytes.read_u128::<LE>()? as u128,
                         _ => unreachable!(),

diff --git a/decoder/src/lib.rs b/decoder/src/lib.rs
@@ -213,9 +213,9 @@ enum Arg<'t> {
     Bool(bool),
     F32(f32),
     F64(f64),
-    /// U8, U16, U24 and U32
+    /// U8, U16, U32, U64, U128
     Uxx(u128),
-    /// I8, I16, I24 and I32
+    /// I8, I16, I32, I64, I128
     Ixx(i128),
     /// Str
     Str(String),
@@ -360,7 +360,7 @@ mod tests {
     #[test]
     fn all_integers() {
         const FMT: &str =
-            "Hello, {=u8} {=u16} {=u24} {=u32} {=u64} {=u128} {=i8} {=i16} {=i32} {=i64} {=i128}!";
+            "Hello, {=u8} {=u16} {=u32} {=u64} {=u128} {=i8} {=i16} {=i32} {=i64} {=i128}!";
         let mut entries = BTreeMap::new();
         entries.insert(0, TableEntry::new_without_symbol(Tag::Info, FMT.to_owned()));
 
@@ -373,7 +373,6 @@ mod tests {
             0, 0,  // index
             42, // u8
             0xff, 0xff, // u16
-            0, 0, 1, // u24
             0xff, 0xff, 0xff, 0xff, // u32
             0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, // u64
             0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
@@ -398,7 +397,6 @@ mod tests {
                     vec![
                         Arg::Uxx(42),                      // u8
                         Arg::Uxx(u16::max_value().into()), // u16
-                        Arg::Uxx(0x10000),                 // u24
                         Arg::Uxx(u32::max_value().into()), // u32
                         Arg::Uxx(u64::max_value().into()), // u64
                         Arg::Uxx(u128::max_value()),       // u128

diff --git a/macros/src/lib.rs b/macros/src/lib.rs
@@ -1155,7 +1155,6 @@ impl Codegen {
 
                 defmt_parser::Type::U8 => exprs.push(quote!(_fmt_.u8(#arg))),
                 defmt_parser::Type::U16 => exprs.push(quote!(_fmt_.u16(#arg))),
-                defmt_parser::Type::U24 => exprs.push(quote!(_fmt_.u24(#arg))),
                 defmt_parser::Type::U32 => exprs.push(quote!(_fmt_.u32(#arg))),
                 defmt_parser::Type::U64 => exprs.push(quote!(_fmt_.u64(#arg))),
                 defmt_parser::Type::U128 => exprs.push(quote!(_fmt_.u128(#arg))),
@@ -1212,8 +1211,7 @@ impl Codegen {
                     match truncated_sz {
                         1 => exprs.push(quote!(_fmt_.u8(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
                         2 => exprs.push(quote!(_fmt_.u16(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
-                        3 => exprs.push(quote!(_fmt_.u24(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
-                        4 => exprs.push(quote!(_fmt_.u32(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
+                        3..=4 => exprs.push(quote!(_fmt_.u32(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
                         5..=8 => exprs.push(quote!(_fmt_.u64(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
                         9..=16 => exprs.push(quote!(_fmt_.u128(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8))))),
                         _ => unreachable!(),

diff --git a/parser/src/lib.rs b/parser/src/lib.rs
@@ -763,15 +763,6 @@ mod tests {
             })
         );
 
-        assert_eq!(
-            parse_param("=u24", ParserMode::Strict),
-            Ok(Param {
-                index: None,
-                ty: Type::U24,
-                hint: None,
-            })
-        );
-
         assert_eq!(
             parse_param("=u32", ParserMode::Strict),
             Ok(Param {

diff --git a/parser/src/types.rs b/parser/src/types.rs
@@ -33,7 +33,6 @@ pub enum Type {
 
     U8,
     U16,
-    U24,
     U32,
     U64,
     U128,
@@ -51,7 +50,6 @@ impl FromStr for Type {
         Ok(match s {
             "u8" => Type::U8,
             "u16" => Type::U16,
-            "u24" => Type::U24,
             "u32" => Type::U32,
             "u64" => Type::U64,
             "u128" => Type::U128,