log: `mw_log_fmt` implementation #13

arkjedrz · 2025-12-01T09:47:50Z

Replacement for core::fmt.
Unit tests.

Notes for Reviewer

Pre-Review Checklist for the PR Author

PR title is short, expressive and meaningful
Commits are properly organized
Relevant issues are linked in the References section
Tests are conducted
Unit tests are added

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Unit tests have been written for new behavior
Public API is documented
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes #

- Replacement for `core::fmt`. - Unit tests.

github-actions · 2025-12-01T09:48:34Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.4.2) and connecting to it...
INFO: Invocation ID: dea8e799-0ee0-45f3-83d9-56eecbfb33d3
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
DEBUG: Rule 'rust_qnx8_toolchain+' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-eQOopREOYCL5vtTb6c1cwZrql4GVrJ1FqgxarQRe1xs="
DEBUG: Repository rust_qnx8_toolchain+ instantiated at:
  <builtin>: in <toplevel>
Repository rule http_archive defined at:
  /home/runner/.bazel/external/bazel_tools/tools/build_defs/repo/http.bzl:431:31: in <toplevel>
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'aspect_rules_lint', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'buildifier_prebuilt', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (25 packages loaded, 9 targets configured)

Analyzing: target //:license-check (80 packages loaded, 9 targets configured)

Analyzing: target //:license-check (135 packages loaded, 1626 targets configured)

Analyzing: target //:license-check (140 packages loaded, 2564 targets configured)

Analyzing: target //:license-check (140 packages loaded, 2564 targets configured)

Analyzing: target //:license-check (151 packages loaded, 4750 targets configured)

INFO: Analyzed target //:license-check (152 packages loaded, 4876 targets configured).
[7 / 14] Creating runfiles tree bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/score_tooling+/dash/tool/formatters/dash_format_converter.runfiles [for tool]; 0s local
INFO: From Generating Dash formatted dependency file ...:
WARNING: No packages found in Cargo.lock.
INFO: Successfully converted 0 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 24.087s, Critical Path: 0.39s
INFO: 14 processes: 4 disk cache hit, 9 internal, 1 processwrapper-sandbox.
INFO: Build completed successfully, 14 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

github-actions · 2025-12-01T09:50:17Z

The created documentation from the pull request is available at: docu-html

awillenbuecher-xq-tec · 2025-12-03T08:23:55Z

src/log/mw_log_fmt/src/macros.rs

+#[macro_export]
+macro_rules! score_write {
+    ($dst:expr, $($arg:tt)*) => {
+        write($dst, mw_log_format_args!($($arg)*))


This needs to be qualified with the expansion of the crate name:

Suggested change

write($dst, mw_log_format_args!($($arg)*))

$crate::write($dst, mw_log_format_args!($($arg)*))

awillenbuecher-xq-tec · 2025-12-03T08:24:08Z

src/log/mw_log_fmt/src/macros.rs

+        $crate::score_write!($dst, "\n")
+    };
+    ($dst:expr, $($arg:tt)*) => {
+        write($dst, mw_log_format_args_nl!($($arg)*))


awillenbuecher-xq-tec · 2025-12-03T08:34:53Z

src/log/mw_log_fmt/src/fmt.rs

+    new_format!(new_debug, ScoreDebug);
+    new_format!(new_display, ScoreDisplay);


Are you planning to call the new_format! macro in more places beyond these two? Declaring a macro just to create two copies of a relatively short method seems like overkill, and makes the code harder to understand (especially because it kills IDE support within the method implementation).

Ye also there will be rule functions over macros. So we shall change that wherever possible.

awillenbuecher-xq-tec · 2025-12-03T08:49:29Z

src/log/mw_log_fmt/src/fmt.rs

+
+macro_rules! new_format {
+    ($name:ident, $trait:ident) => {
+        pub const fn $name<T: $trait>(value: &T, spec: FormatSpec) -> Self {


This fails to capture the lifetime of &T under 'a, which lets other code mutate value while Placeholder conceptionally contains a reference to value. Example:

let mut value = 5_u32; let ph = Fragment::Placeholder(Placeholder::new_display(&value, FormatSpec::new())); value = 6; // This must be prevented! mw_log_fmt::write(&mut buffer, Arguments(&[ph])).unwrap(); // This will write "6"

Constrain &T to fix this:

Suggested change

pub const fn $name<T: $trait>(value: &T, spec: FormatSpec) -> Self {

pub const fn $name<T: $trait>(value: &'a T, spec: FormatSpec) -> Self {

do we have more insight why this works like that ?

The compiler accepts the old version, because the body of the function erases the lifetime of the reference value: &'b T by converting it to a pointer. The closure within the function then effectively converts this pointer to a reference &'a T, which is wrong, but the compiler doesn't know that.

When doing reference-pointer-reference roundtrips, you must be very careful with lifetime annotations in function signatures and on type declarations, because the compiler won't complain if you make a mistake.

awillenbuecher-xq-tec · 2025-12-03T09:09:23Z

src/log/mw_log_fmt/src/fmt.rs

+pub trait ScoreWrite {
+    fn write_bool(&mut self, v: &bool, spec: &FormatSpec) -> Result;
+    fn write_f32(&mut self, v: &f32, spec: &FormatSpec) -> Result;
+    fn write_f64(&mut self, v: &f64, spec: &FormatSpec) -> Result;
+    fn write_i8(&mut self, v: &i8, spec: &FormatSpec) -> Result;
+    fn write_i16(&mut self, v: &i16, spec: &FormatSpec) -> Result;
+    fn write_i32(&mut self, v: &i32, spec: &FormatSpec) -> Result;
+    fn write_i64(&mut self, v: &i64, spec: &FormatSpec) -> Result;
+    fn write_u8(&mut self, v: &u8, spec: &FormatSpec) -> Result;
+    fn write_u16(&mut self, v: &u16, spec: &FormatSpec) -> Result;
+    fn write_u32(&mut self, v: &u32, spec: &FormatSpec) -> Result;
+    fn write_u64(&mut self, v: &u64, spec: &FormatSpec) -> Result;
+    fn write_str(&mut self, v: &str, spec: &FormatSpec) -> Result;
+}


This architecture requires that every implementer of ScoreWrite needs to handle every supported type. That's a lot of duplication.

Why do it like this, instead of taking the approach in core::fmt::Write? That trait only requires implementing write_str, and it provides a predefined write_fmt convenience method.

This is a requirement from S-CORE. Basically, the Log backend is using type information to pack it with some specific formats, ie DLT. So there is no othe way like live this to back which can decide what to do with it.

pawelrutkaq · 2025-12-03T07:10:46Z

src/log/BUILD

@@ -1,24 +0,0 @@
-# *******************************************************************************


no src structs inside component modules. (use custom lib.rs path in cargo toml.)

pawelrutkaq · 2025-12-03T07:11:32Z

src/log/mw_log_fmt/src/fmt.rs

+//
+
+use crate::FormatSpec;
+use core::fmt::Error as CoreFmtError;


do we need this ? this is not vcertified as of now.

pawelrutkaq · 2025-12-03T07:13:06Z

src/log/mw_log_fmt/src/fmt.rs

+    }
+}
+
+pub trait ScoreWrite {


pawelrutkaq · 2025-12-03T07:15:28Z

src/log/mw_log_fmt/src/fmt.rs

+    fn write_str(&mut self, v: &str, spec: &FormatSpec) -> Result;
+}
+
+#[derive(Debug)]


We shall not use core::fmt in our fmt ;)

pawelrutkaq · 2025-12-03T07:17:08Z

src/log/mw_log_fmt/src/fmt.rs

+        &self.spec
+    }
+
+    pub fn fmt(&self, f: &mut dyn ScoreWrite, spec: &FormatSpec) -> Result {


hide &mut dyn ScoreWrite this after some wrapping type with deref for now if needed

pawelrutkaq · 2025-12-03T07:35:00Z

src/log/mw_log_fmt/src/fmt_impl.rs

+            $fmt::fmt(&v, &mut w, &FormatSpec::new()).unwrap();
+            assert_eq!(w.get(), format!("{v}"));
+        };
+    }


i think you could use a generic fn instead macro or ?

pawelrutkaq · 2025-12-04T11:47:33Z

src/log/mw_log_fmt/src/fmt.rs

+    new_format!(new_debug, ScoreDebug);
+    new_format!(new_display, ScoreDisplay);


Ye also there will be rule functions over macros. So we shall change that wherever possible.

pawelrutkaq · 2025-12-04T11:48:55Z

src/log/mw_log_fmt/src/fmt.rs

+
+macro_rules! new_format {
+    ($name:ident, $trait:ident) => {
+        pub const fn $name<T: $trait>(value: &T, spec: FormatSpec) -> Self {


do we have more insight why this works like that ?

pawelrutkaq · 2025-12-04T11:50:27Z

src/log/mw_log_fmt/src/fmt.rs

+pub trait ScoreWrite {
+    fn write_bool(&mut self, v: &bool, spec: &FormatSpec) -> Result;
+    fn write_f32(&mut self, v: &f32, spec: &FormatSpec) -> Result;
+    fn write_f64(&mut self, v: &f64, spec: &FormatSpec) -> Result;
+    fn write_i8(&mut self, v: &i8, spec: &FormatSpec) -> Result;
+    fn write_i16(&mut self, v: &i16, spec: &FormatSpec) -> Result;
+    fn write_i32(&mut self, v: &i32, spec: &FormatSpec) -> Result;
+    fn write_i64(&mut self, v: &i64, spec: &FormatSpec) -> Result;
+    fn write_u8(&mut self, v: &u8, spec: &FormatSpec) -> Result;
+    fn write_u16(&mut self, v: &u16, spec: &FormatSpec) -> Result;
+    fn write_u32(&mut self, v: &u32, spec: &FormatSpec) -> Result;
+    fn write_u64(&mut self, v: &u64, spec: &FormatSpec) -> Result;
+    fn write_str(&mut self, v: &str, spec: &FormatSpec) -> Result;
+}


This is a requirement from S-CORE. Basically, the Log backend is using type information to pack it with some specific formats, ie DLT. So there is no othe way like live this to back which can decide what to do with it.

pawelrutkaq · 2025-12-04T11:53:00Z

src/log/mw_log_fmt/src/fmt_spec.rs

+// SPDX-License-Identifier: Apache-2.0
+//
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]


this shall be also ScoreDebug or ?

log: mw_log_fmt implementation

119fa99

- Replacement for `core::fmt`. - Unit tests.

awillenbuecher-xq-tec suggested changes Dec 3, 2025

View reviewed changes

pawelrutkaq suggested changes Dec 4, 2025

View reviewed changes

	write($dst, mw_log_format_args!($($arg)*))
	$crate::write($dst, mw_log_format_args!($($arg)*))

		new_format!(new_debug, ScoreDebug);
		new_format!(new_display, ScoreDisplay);

	pub const fn $name<T: $trait>(value: &T, spec: FormatSpec) -> Self {
	pub const fn $name<T: $trait>(value: &'a T, spec: FormatSpec) -> Self {

		@@ -1,24 +0,0 @@
		# *******************************************************************************

log: mw_log_fmt implementation #13

Are you sure you want to change the base?

log: mw_log_fmt implementation #13

Uh oh!

Conversation

arkjedrz commented Dec 1, 2025

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Uh oh!

github-actions bot commented Dec 1, 2025

License Check Results

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awillenbuecher-xq-tec Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

log: `mw_log_fmt` implementation #13

log: `mw_log_fmt` implementation #13

awillenbuecher-xq-tec Dec 4, 2025 •

edited

Loading