std::format instead of snprintf in logger.cpp by yotam-medini · Pull Request #4767 · canonical/mir

yotam-medini · 2026-03-11T14:39:54Z

Closes nothing

Related: #4660

Related to #4736

What's new?

std::format instead of snprintf in logger.cpp

How to test

Checklist

Tests added and pass
Adequate documentation added
(optional) Added Screenshots or videos

Copilot

Pull request overview

This PR updates Mir’s core logger timestamp formatting to use C++ std::format/std::chrono instead of snprintf-based char buffers, aligning with the direction in issue #4660.

Changes:

Replace manual timespec/strftime/snprintf timestamp construction with std::chrono + std::format.
Add a fallback timestamp path when time zone formatting via zoned_time/current_zone() fails.
Build a preformatted "[timestamp] <severity>" prefix using std::format_to_n.

AlanGriffiths

I know this idea originates from one of my comments, but I don't think that should be applied without regard to context.

Specifically, I'm not sure the change is well motivated here. The proposed code is harder to follow and adds complexity to widely used functionality. Is there anything it makes better?

Revert to UTC if current_zone() throws exception.

AlanGriffiths

I'm not sure the change is well motivated here:

A significant part of the motivation to move away from snprintf() is the fragile nature of passing char*, size_t pairs. But with this proposal we still end up doing the same thing for std::format_to_n().
The proposed code is harder to follow and adds complexity to widely used functionality.

Is there anything it makes better?

yotam-medini · 2026-03-18T12:45:33Z

Is there anything it makes better?

In my recent commit I tried to avoid explicit passing the size-limit
in "our" mir functions (via TimeStampBuffer).
As I understood from previous review comments, in ml::format_message
we want to avoid dynamic allocation, so exceptions will not be thrown.
Thus we cannot use std::format(...) and use automatic buffer on the stack.

If we want to move to modern C++ API as reasonably possible,
Can you suggest an alternative to C++ format-API?

Or, would you suggest to give up on eliminating snprintf
from ml::format_message(...) in logger.cpp?

AlanGriffiths · 2026-03-18T14:31:24Z

Is there anything it makes better?

In my recent commit I tried to avoid explicit passing the size-limit in "our" mir functions (via TimeStampBuffer). As I understood from previous review comments, in ml::format_message we want to avoid dynamic allocation, so exceptions will not be thrown. Thus we cannot use std::format(...) and use automatic buffer on the stack.

Yes, we've explored alternatives: None of these appear to be a clear improvement to the existing code.

If we want to move to modern C++ API as reasonably possible, Can you suggest an alternative to C++ format-API?

We seem to have different objectives: I want to improve the code, you want to "move to modern C++ API as reasonably possible". While new APIs and features are, in general, good options they are not always an improvement.

Or, would you suggest to give up on eliminating snprintf from ml::format_message(...) in logger.cpp?

I'm open to being convinced some variant of this is an improvement. But:

one call to snprintf() seems simpler than two calls to format_to_n() (and some logic);
snprintf() and format_to_n() have the same memory safety issue; and,
format(...) leads to potential allocations with possible performance or exceptions being raised.

yotam-medini · 2026-03-18T15:40:40Z

Before coding.
How about avoiding char buffers and use something like:

   // no (char*,size) pairs, direct write to the stream
   auto iter = std::ostreambuf_iterator<char>(out);
   iter = std::format_to(iter, "{:%F %T}", ...);
   [iter =] std::format_to(iter, " {}  ...", severity, ...);
   ...
   out << std::endl; // for flush

While at it. When I tested the try/catch section in "my" format_timestamp(...)
I wanted to fail the current_zone() and have the exception caught.
By playing with env-vars ("TZ", "TZINFO"} or [re]moving system files
/etc/localtime, or under /usr/share/zoneinfo,
but current_zone() overcame all such "landmines".
Eventually, I just added temporary throw for testing.

How about breaking backward compatibility, and just use:
... , "{:%F %T} UTC", now_us)

robert-ancell

I've no further issues with this PR but I'll leave it to @AlanGriffiths for the final approval as this is using C++ APIs I'm not familiar with.

AlanGriffiths · 2026-03-19T12:10:46Z

Before coding. How about avoiding char buffers and use something like:
...
iter = std::format_to(iter, "{:%F %T}", ...);

That would avoid both allocating a string and passing ..., buf, len, ...; and also looks like it will actually compile with g++-13. So that could actually be an improvement.

While at it. When I tested the try/catch section in "my" format_timestamp(...) I wanted to fail the current_zone() and have the exception caught.

Maybe that points to a design issue with format_timestamp(...)? The try-catch logic could, in principle, be separated out. But this logic is too elaborate: if the runtime cannot find any tzdb, the environment is likely toxic anyway. And throwing and catching an exception for every log message is expensive. Maybe:

void format_timestamp(...)
try
{
    ...
}
catch (std::runtime_error const& e)
{
    mir::fatal_error_abort("Cannot format timestamp: %s", e.what());
}

AlanGriffiths

Looks promising. Just needs some tidy-up. Something like:

    try
    {
        std::ostreambuf_iterator<char> iter{out};
        auto const now = std::chrono::system_clock::now();
        auto const now_us = std::chrono::time_point_cast<std::chrono::microseconds>(now);
        iter = std::format_to(iter, "[{:%F %T}]{}{}: {}\n",
            std::chrono::zoned_time{std::chrono::current_zone(), now_us},
            lut[static_cast<int>(severity)], component, message);
        out.flush();
    }
    catch (std::runtime_error const& e)
    {
        mir::fatal_error_abort("Cannot format log message: %s", e.what());
    }

AlanGriffiths · 2026-03-20T11:15:56Z

 #include <cstdio>
 #include <ctime>


I don't think we need cstdio or ctime anymore

We still need , since logger.cpp still uses vsnprintf(...)
that is not addressed in this task.

AlanGriffiths · 2026-03-20T11:17:03Z

+namespace {
+
+using OutIter = std::ostreambuf_iterator<char>;
+
+OutIter format_timestamp(OutIter iter)
+{
+    auto now = std::chrono::system_clock::now();
+    auto now_us = std::chrono::time_point_cast<std::chrono::microseconds>(now);
+    try {
+        auto local =
+            std::chrono::zoned_time{std::chrono::current_zone(), now_us};
+        iter = std::format_to(iter, "[{:%F %T}]", local);
+    } catch (std::runtime_error const& e) {
+        mir::fatal_error_abort("Cannot format timestamp: %s", e.what());
+    }
+    return iter;
+}
+}


I think this can all be inlined - it is simple enough to be part of ml::format_message()

AlanGriffiths · 2026-03-20T11:18:26Z

+    iter = format_timestamp(iter);
+    format_to(iter, "{}{}: {}\n",
+        lut[static_cast<int>(severity)], component, message);


After inlining format_timestamp(), the format_to() statements can be combined into a single call.

single call now.

AlanGriffiths · 2026-03-20T11:19:16Z

+    try {
+        auto local =
+            std::chrono::zoned_time{std::chrono::current_zone(), now_us};
+        iter = std::format_to(iter, "[{:%F %T}]", local);
+    } catch (std::runtime_error const& e) {
+        mir::fatal_error_abort("Cannot format timestamp: %s", e.what());
+    }


According to the style guide the braces should be on their own lines.

In recent commit:
c9461d2 yotam.m inline format_timestamp, single std::format_to call(...)
I tried to follow your suggection.
Two remarks.

My earlier separation of format_timestamp(...) was following
Robert's suggesion from March 11.

In general, don't we want to localize try/catch block
to the smallest possible code, for which exception may be thrown?
In this case: std::chrono::current_zone().

It is simpler now than when Robert suggested separating it

That depends on how it affects the complexity of the code. We could write:

auto const local = [] { try { return std::chrono::zoned_time{std::chrono::current_zone(), now_us}; } catch (std::runtime_error const& e) { mir::fatal_error_abort("Cannot format log timestamp: %s", e.what()); } }();

But that's an absurd extreme.

yotam-medini · 2026-03-20T17:01:32Z

BTW, how can I run a single failed CI test. For example: the current single failed:
Spread / BuildAndTest (lxd:...:spread/ubuntu:asan) (pull_request)Failing after 4m
?

AlanGriffiths

A little tweaking, but looks good.

Maybe a strategic using namespace std::chrono; would allow us to reduce unnecessary verbosity?

AlanGriffiths · 2026-03-20T17:04:02Z

+        std::ostreambuf_iterator<char> iter{out};
+        auto now = std::chrono::system_clock::now();
+        auto now_us =
+            std::chrono::time_point_cast<std::chrono::microseconds>(now);
+        auto local =
+            std::chrono::zoned_time{std::chrono::current_zone(), now_us};
+        std::format_to(iter, "[{:%F %T}] {}{}: {}\n",
+            local, lut[static_cast<int>(severity)], component, message);
+        out.flush();


We don't really need a name for iter:

Suggested change

std::ostreambuf_iterator<char> iter{out};

auto now = std::chrono::system_clock::now();

auto now_us =

std::chrono::time_point_cast<std::chrono::microseconds>(now);

auto local =

std::chrono::zoned_time{std::chrono::current_zone(), now_us};

std::format_to(iter, "[{:%F %T}] {}{}: {}\n",

local, lut[static_cast<int>(severity)], component, message);

out.flush();

auto now = std::chrono::system_clock::now();

auto now_us =

std::chrono::time_point_cast<std::chrono::microseconds>(now);

auto local =

std::chrono::zoned_time{std::chrono::current_zone(), now_us};

std::format_to(std::ostreambuf_iterator{out}, "[{:%F %T}] {}{}: {}\n",

local, lut[static_cast<int>(severity)], component, message);

out.flush();

AlanGriffiths · 2026-03-20T17:05:50Z

+    }
+    catch (std::runtime_error const& e)
+    {
+        mir::fatal_error_abort("Cannot format message: %s", e.what());


This message is a little too generic: we ought to mention logging so that the reader has some idea where it comes from:

Suggested change

mir::fatal_error_abort("Cannot format message: %s", e.what());

mir::fatal_error_abort("Cannot format log message: %s", e.what());

AlanGriffiths · 2026-03-20T17:09:05Z

 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

+#include <mir/fatal.h>


Project standard is to begin a .cpp file with the corresponding .h (to ensure the .h has no hidden dependencies).

AlanGriffiths · 2026-03-20T17:18:37Z

BTW, how can I run a single failed CI test. For example: the current single failed: Spread / BuildAndTest (lxd:...:spread/ubuntu:asan) (pull_request)Failing after 4m ?

I doubt you have the permissions to re-run CI tasks. In this case it looks like a network timeout, nothing to do with your changes.

Team members (like me) could re-run it, but as I've requested changes there's no point.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

AlanGriffiths

I still think we can inline iter, but ok

AlanGriffiths · 2026-03-23T09:54:49Z

+        auto now = system_clock::now();
+        auto now_us = time_point_cast<microseconds>(now);
+        auto local = zoned_time{current_zone(), now_us};
+        std::format_to(std::ostreambuf_iterator<char>(out),


It's only six characters, but the compiler can deduce the template parameter.

Suggested change

std::format_to(std::ostreambuf_iterator<char>(out),

std::format_to(std::ostreambuf_iterator(out),

AlanGriffiths

The doc check is failing because www.akkadia.org has dropped off the internet. (So non-blocking on this PR)

yotam-medini · 2026-03-23T10:12:42Z

documentation-checks ... failed with:
matplotlib is not installed, social cards will not be generated
<rst_epilog>:2: CRITICAL: Problems with "include" directive path:
InputError: [Errno 2] No such file or directory: 'reuse/links.txt'.
<rst_epilog>:3: CRITICAL: Problems with "include" directive path:
InputError: [Errno 2] No such file or directory: 'reuse/substitutions.txt'.

Do I need to do anything to have this PR merged ?

AlanGriffiths · 2026-03-23T10:14:16Z

Do I need to do anything to have this PR merged

No, I'm dealing with it

std::format instead of snprintf in logger.cpp

4053059

yotam-medini requested a review from a team as a code owner March 11, 2026 14:39

Saviq requested a review from Copilot March 11, 2026 16:20

Copilot started reviewing on behalf of Saviq March 11, 2026 16:21 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Comment thread src/core/logging/logger.cpp Outdated

Comment thread src/core/logging/logger.cpp Outdated

Comment thread src/core/logging/logger.cpp Outdated

Comment thread src/core/logging/logger.cpp Outdated

yotam-medini added 2 commits March 11, 2026 19:57

std::format_to_n(...) leave safe place for the null char

6463688

localtime_r, yyyy-mm-dd

86c33fb

robert-ancell reviewed Mar 11, 2026

View reviewed changes

Comment thread src/core/logging/logger.cpp

added format_message() funcion

eb13288

yotam-medini requested a review from robert-ancell March 12, 2026 04:32

AlanGriffiths reviewed Mar 12, 2026

View reviewed changes

Comment thread src/core/logging/logger.cpp Outdated

Comment thread src/core/logging/logger.cpp Outdated

rewrite of format_timestamp(...), no allocation

fe3781a

Revert to UTC if current_zone() throws exception.

yotam-medini requested a review from AlanGriffiths March 12, 2026 20:17

AlanGriffiths reviewed Mar 13, 2026

View reviewed changes

Comment thread src/core/logging/logger.cpp Outdated

using TimeStampBuffer = std::array<char, 64>

6302e68

yotam-medini requested a review from AlanGriffiths March 14, 2026 00:34

AlanGriffiths requested changes Mar 18, 2026

View reviewed changes

robert-ancell reviewed Mar 18, 2026

View reviewed changes

yotam-medini requested a review from AlanGriffiths March 19, 2026 07:18

std::format_to(ostreambuf_iterator,..) no TimeStampBuffer

73a3fa8

AlanGriffiths requested changes Mar 20, 2026

View reviewed changes

inline format_timestamp, single std::format_to call(...)

c9461d2

yotam-medini requested a review from AlanGriffiths March 20, 2026 16:22

AlanGriffiths requested changes Mar 20, 2026

View reviewed changes

AlanGriffiths reviewed Mar 20, 2026

View reviewed changes

inc-header moved down, better err-msg, using namespace chrono

f0ca769

yotam-medini requested a review from AlanGriffiths March 20, 2026 18:43

AlanGriffiths requested a review from Copilot March 21, 2026 00:19

Copilot started reviewing on behalf of AlanGriffiths March 21, 2026 00:25 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

Comment thread src/core/logging/logger.cpp

include <cerrno>

998bdf0

AlanGriffiths approved these changes Mar 22, 2026

View reviewed changes

yotam-medini added 2 commits March 22, 2026 23:05

Merge branch 'main' into stdfmt-4660-logger

0685f0b

inline iter

40ff454

AlanGriffiths approved these changes Mar 23, 2026

View reviewed changes

AlanGriffiths enabled auto-merge March 23, 2026 09:55

AlanGriffiths approved these changes Mar 23, 2026

View reviewed changes

ostreambuf_iterator deduce template arg

3f91ec5

auto-merge was automatically disabled March 23, 2026 10:43
Head branch was pushed to by a user without write access

AlanGriffiths merged commit f984ec7 into canonical:main Mar 23, 2026
50 of 52 checks passed

	mir::fatal_error_abort("Cannot format message: %s", e.what());
	mir::fatal_error_abort("Cannot format log message: %s", e.what());

	std::format_to(std::ostreambuf_iterator<char>(out),
	std::format_to(std::ostreambuf_iterator(out),

Conversation

yotam-medini commented Mar 11, 2026

What's new?

How to test

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

yotam-medini commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanGriffiths commented Mar 18, 2026

Uh oh!

yotam-medini commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robert-ancell left a comment

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths commented Mar 19, 2026

Uh oh!

AlanGriffiths left a comment

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!

AlanGriffiths Mar 20, 2026 • 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!

yotam-medini commented Mar 20, 2026

Uh oh!

AlanGriffiths left a comment

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!

AlanGriffiths commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

yotam-medini commented Mar 18, 2026 •

edited

Loading

yotam-medini commented Mar 18, 2026 •

edited

Loading

AlanGriffiths Mar 20, 2026 •

edited

Loading

yotam-medini commented Mar 23, 2026 •

edited

Loading