Repository: knurling-rs/defmt
Pull request #524 (2021-06-30 01:14:48)

[4/n] test: do not depend on custom InternalFormatter
Part of #492

#508 removes InternalFormatter, so it will no longer be possible to store a `Vec<u8>` there for fake-writing the bytes for testing. This PR replaces that with writing a thread-local `Vec<u8>`.


Patch:
diff --git a/macros/src/lib.rs b/macros/src/lib.rs
index 1f7cedd..e9332cd 100644
--- a/macros/src/lib.rs
+++ b/macros/src/lib.rs
@@ -169,47 +169,29 @@ pub fn timestamp(ts: TokenStream) -> TokenStream {
         Err(e) => return e.to_compile_error().into(),
     };
 
-    if cfg!(feature = "unstable-test") {
-        // Only check that the formatting arguments compile.
-        quote!(
-            const _: () = {
-                #[allow(unused)]
-                fn defmt_timestamp(fmt: ::defmt::Formatter<'_>) {
-                    match (fmt.inner, #(&(#args)),*) {
-                        (_fmt_, #(#pats),*) => {
-                            // NOTE: No format string index, and no finalize call.
-                            #(#exprs;)*
-                        }
-                    }
-                }
-            };
-        )
-        .into()
-    } else {
-        quote!(
-            const _: () = {
-                #[export_name = "_defmt_timestamp"]
-                fn defmt_timestamp(fmt: ::defmt::Formatter<'_>) {
-                    match (fmt.inner, #(&(#args)),*) {
-                        (_fmt_, #(#pats),*) => {
-                            // NOTE: No format string index, and no finalize call.
-                            #(#exprs;)*
-                        }
+    quote!(
+        const _: () = {
+            #[export_name = "_defmt_timestamp"]
+            fn defmt_timestamp(fmt: ::defmt::Formatter<'_>) {
+                match (fmt.inner, #(&(#args)),*) {
+                    (_fmt_, #(#pats),*) => {
+                        // NOTE: No format string index, and no finalize call.
+                        #(#exprs;)*
                     }
                 }
+            }
 
-                #sym;
+            #sym;
 
-                // Unique symbol name to prevent multiple `timestamp!` invocations in the crate graph.
-                // Uses `#symname` to ensure it is not discarded by the linker.
-                #[no_mangle]
-                #[cfg_attr(target_os = "macos", link_section = ".defmt,end.timestamp")]
-                #[cfg_attr(not(target_os = "macos"), link_section = ".defmt.end.timestamp")]
-                static __DEFMT_MARKER_TIMESTAMP_WAS_DEFINED: &u8 = &#symname;
-            };
-        )
-        .into()
-    }
+            // Unique symbol name to prevent multiple `timestamp!` invocations in the crate graph.
+            // Uses `#symname` to ensure it is not discarded by the linker.
+            #[no_mangle]
+            #[cfg_attr(target_os = "macos", link_section = ".defmt,end.timestamp")]
+            #[cfg_attr(not(target_os = "macos"), link_section = ".defmt.end.timestamp")]
+            static __DEFMT_MARKER_TIMESTAMP_WAS_DEFINED: &u8 = &#symname;
+        };
+    )
+    .into()
 }
 
 /// Returns a list of features of which one has to be enabled for `level` to be active
diff --git a/src/export.rs b/src/export.rs
index bb297ca..aef94c4 100644
--- a/src/export.rs
+++ b/src/export.rs
@@ -2,10 +2,8 @@ use crate::Str;
 
 #[cfg(feature = "unstable-test")]
 thread_local! {
-    static I: core::sync::atomic::AtomicU16 =
-        core::sync::atomic::AtomicU16::new(0);
-    static T: core::sync::atomic::AtomicU16 =
-        core::sync::atomic::AtomicU16::new(0);
+    static I: core::sync::atomic::AtomicU16 = core::sync::atomic::AtomicU16::new(0);
+    static BYTES: core::cell::RefCell<Vec<u8>> = core::cell::RefCell::new(Vec::new());
 }
 
 /// For testing purposes
@@ -20,6 +18,12 @@ pub fn fetch_add_string_index() -> usize {
     (I.with(|i| i.fetch_add(1, core::sync::atomic::Ordering::Relaxed))) as usize
 }
 
+/// Get and clear the logged bytes
+#[cfg(feature = "unstable-test")]
+pub fn fetch_bytes() -> Vec<u8> {
+    BYTES.with(|b| core::mem::replace(&mut *b.borrow_mut(), Vec::new()))
+}
+
 #[cfg(feature = "unstable-test")]
 pub fn acquire() {}
 
@@ -44,6 +48,11 @@ pub fn release() {
     unsafe { _defmt_release() }
 }
 
+#[cfg(feature = "unstable-test")]
+pub fn write(bytes: &[u8]) {
+    BYTES.with(|b| b.borrow_mut().extend(bytes))
+}
+
 #[cfg(not(feature = "unstable-test"))]
 #[inline(never)]
 pub fn write(bytes: &[u8]) {
diff --git a/src/formatter.rs b/src/formatter.rs
index e90f83b..52462e8 100644
--- a/src/formatter.rs
+++ b/src/formatter.rs
@@ -11,8 +11,6 @@ pub struct Formatter<'a> {
 
 #[doc(hidden)]
 pub struct InternalFormatter {
-    #[cfg(feature = "unstable-test")]
-    bytes: Vec<u8>,
     /// Whether to omit the tag of a `Format` value
     ///
     /// * this is disabled while formatting a `{:[?]}` value (second element on-wards)
@@ -22,7 +20,6 @@ pub struct InternalFormatter {
 
 #[doc(hidden)]
 impl InternalFormatter {
-    #[cfg(not(feature = "unstable-test"))]
     pub fn write(&mut self, bytes: &[u8]) {
         export::write(bytes)
     }
@@ -32,7 +29,6 @@ impl InternalFormatter {
     /// # Safety
     ///
     /// Must only be called when the current execution context has acquired the logger with [Logger::acquire]
-    #[cfg(not(feature = "unstable-test"))]
     #[allow(clippy::new_without_default)]
     pub fn new() -> Self {
         Self { omit_tag: false }
@@ -241,23 +237,3 @@ impl fmt::Write for FmtWrite<'_> {
         Ok(())
     }
 }
-
-#[doc(hidden)]
-#[cfg(feature = "unstable-test")]
-impl super::InternalFormatter {
-    #[allow(clippy::new_without_default)]
-    pub fn new() -> Self {
-        Self {
-            bytes: vec![],
-            omit_tag: false,
-        }
-    }
-
-    pub fn bytes(&mut self) -> &[u8] {
-        &self.bytes
-    }
-
-    pub fn write(&mut self, bytes: &[u8]) {
-        self.bytes.extend_from_slice(bytes)
-    }
-}


Test patch:
diff --git a/tests/encode.rs b/tests/encode.rs
index 45fca3c..36dff2f 100644
--- a/tests/encode.rs
+++ b/tests/encode.rs
@@ -21,11 +21,11 @@
 // let mut f = InternalFormatter::new();
 // let index = defmt::export::fetch_string_index();
 // foo(&mut f); // NOTE increases the interner index
-// assert_eq!(f.bytes(), [index]);
+// assert_eq!(fetch_bytes(), [index]);
 //
 // let mut f = InternalFormatter::new();
 // foo(&mut f);
-// assert_eq!(f.bytes(), [index.wrapping_add(1)]);
+// assert_eq!(fetch_bytes(), [index.wrapping_add(1)]);
 //                               ^^^^^^^^^^^^^^^ account for the previous `foo` call
 // ```
 //
@@ -50,17 +50,17 @@ fn check_format_implementation(val: &(impl Format + ?Sized), expected_encoding:
     let mut f = InternalFormatter::new();
     let g = Formatter { inner: &mut f };
     val.format(g);
-    assert_eq!(f.bytes(), expected_encoding);
+    assert_eq!(defmt::export::fetch_bytes(), expected_encoding);
 }
 
 macro_rules! check {
-    ($got:expr, [$($x:expr),* $(,)?]) => {
+    ([$($x:expr),* $(,)?]) => {
         {
             let mut v = Vec::<u8>::new();
             $(
                 v.extend(&($x).to_le_bytes());
             )*
-            assert_eq!($got, &v);
+            assert_eq!(defmt::export::fetch_bytes(), v);
         }
     };
 }
@@ -84,25 +84,19 @@ fn write() {
     let g = Formatter { inner: f };
 
     write!(g, "The answer is {=u8}", 42);
-    check!(
-        f.bytes(),
-        [
-            index, // "The answer is {=u8}",
-            42u8,  // u8 value
-        ]
-    );
+    check!([
+        index, // "The answer is {=u8}",
+        42u8,  // u8 value
+    ]);
 
     let f2 = &mut InternalFormatter::new();
     let g2 = Formatter { inner: f2 };
     write!(g2, "The answer is {=?}", 42u8);
-    check!(
-        f2.bytes(),
-        [
-            inc(index, 1), // "The answer is {=?}"
-            inc(index, 2), // "{=u8}" / impl Format for u8
-            42u8,          // u8 value
-        ]
-    );
+    check!([
+        inc(index, 1), // "The answer is {=?}"
+        inc(index, 2), // "{=u8}" / impl Format for u8
+        42u8,          // u8 value
+    ]);
 }
 
 #[test]
@@ -116,15 +110,12 @@ fn bitfields_mixed() {
         "bitfields {0=7..12}, {1=0..5}",
         0b1110_0101_1111_0000u16, 0b1111_0000u8
     );
-    check!(
-        f.bytes(),
-        [
-            index, // bitfields {0=7..12}, {1=0..5}",
-            0b1111_0000u8,
-            0b1110_0101u8, // u16
-            0b1111_0000u8, // u8
-        ]
-    );
+    check!([
+        index, // bitfields {0=7..12}, {1=0..5}",
+        0b1111_0000u8,
+        0b1110_0101u8, // u16
+        0b1111_0000u8, // u8
+    ]);
 }
 
 #[test]
@@ -134,14 +125,11 @@ fn bitfields_across_octets() {
     let g = Formatter { inner: f };
 
     write!(g, "bitfields {0=0..7} {0=9..14}", 0b0110_0011_1101_0010u16);
-    check!(
-        f.bytes(),
-        [
-            index, // bitfields {0=0..7} {0=9..14}",
-            0b1101_0010u8,
-            0b0110_0011u8, // u16
-        ]
-    );
+    check!([
+        index, // bitfields {0=0..7} {0=9..14}",
+        0b1101_0010u8,
+        0b0110_0011u8, // u16
+    ]);
 }
 
 #[test]
@@ -155,13 +143,10 @@ fn bitfields_truncate_lower() {
         "bitfields {0=9..14}",
         0b0000_0000_0000_1111_0110_0011_1101_0010u32
     );
-    check!(
-        f.bytes(),
-        [
-            index,         // bitfields {0=9..14}",
-            0b0110_0011u8, // the first octet should have been truncated away
-        ]
-    );
+    check!([
+        index,         // bitfields {0=9..14}",
+        0b0110_0011u8, // the first octet should have been truncated away
+    ]);
 }
 
 #[test]
@@ -171,13 +156,10 @@ fn bitfields_assert_range_exclusive() {
     let g = Formatter { inner: f };
 
     write!(g, "bitfields {0=6..8}", 0b1010_0101u8,);
-    check!(
-        f.bytes(),
-        [
-            index, // "bitfields {0=6..8}"
-            0b1010_0101u8
-        ]
-    );
+    check!([
+        index, // "bitfields {0=6..8}"
+        0b1010_0101u8
+    ]);
 }
 
 #[test]