task-4660 std::format instead of snprintf(buffer...) in 9 files by yotam-medini · Pull Request #4736 · canonical/mir

yotam-medini · 2026-03-02T12:42:08Z

It may close #4660.

What's new?

This commit replaces all calls of snprintf(buffer...) with automatic buffer with std::format.

Use of vsnprintf(...) is not changed. Moving from vsnprinf to std::format requires design decisions.

How to test

Under build directory, I ran:
ctest --exclude-regex wlcs --output-on-failure --stop-on-failure
And it ran successfully. Last message lines:

19/19 Test #19: mir_unit_tests_virtual ..................   Passed    0.38 sec
100% tests passed, 0 tests failed out of 19
Total Test time (real) = 234.74 sec

Checklist

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

RAOF · 2026-03-04T07:00:03Z

src/core/logging/logger.cpp

+    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::string fmt_now = std::format("{:%F %T}", local);


I'm fairly certain this is missing the old 6-digit nsec field?

now_ft.cpp
yotam@zorani:ym-misc 2503:$ g++ -Wall -g --std=c++20 -o /tmp/now_ft now_ft.cpp
yotam@zorani:ym-misc 2504:$ /tmp/now_ft
fmt_now=2026-03-04 12:01:07.958230

RAOF

This looks reasonable, with the exception of the log timestamp format that I'm not sure about. (I shall check!)

AlanGriffiths

While there are advantages to std::format() and std::print() they are not always appropriate. Which means every change site needs to be examined carefully: Is it noexcept? Is it async-signal-safe? Is it performance critical?

AFAICS there's only one place where this is an issue here, but I would err to the cautious side: not improving things where there is any doubt.

There are a couple of places where the narrow window of changes proposed could be expanded to incorporate improvements to the surrounding code.

AlanGriffiths · 2026-03-04T11:00:40Z

src/core/logging/logger.cpp

+    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::string fmt_now = std::format("{:%F %T}", local);


Rather than creating a std::string via std::format() it would be more efficient and clearer to format directly to out using std::print() (replacing the current streaming operator useage).

Did most of this, except that std::print does not work with std::ostream

std::print does not work with std::ostream

It should: https://en.cppreference.com/w/cpp/io/basic_ostream/print.html

I guess it was missing with g++-13:

Formatted output P2093R14 14.1 __cpp_lib_print >= 202207L

A pity you're looking just now: In a couple of months we will be able to use it as we will switch our baseline support to Ubuntu 26.04 (g++-15)

I missed that. I was looking at:
print
This std::print(ostream, ...) is indeed in C++-23 standard but current g++ does not support it

AlanGriffiths · 2026-03-04T11:06:41Z

src/server/frontend_xwayland/xwayland_spawner.cpp

+    std::string path;
+    path = std::format(x11_lock_fmt, xdisplay);
+    unlink(path.c_str());
+    path = std::format(x11_socket_fmt, xdisplay);
+    unlink(path.c_str());


I know it is paranoia: but I feel uncomfortable allocating memory in noexcept code (like a destructor).

("Paranoia" because on any system this code is likely to run on we won't get bad_alloc - memory will be overallocated and using it will kill the processes.)

I know it would mean restructuring the code somewhat, but couldn't we cache the paths in the constructor so that they are only formatted in one place?

For example: introducing a class that formats (and remembers) these paths while providing the Fd would consolidate this logic.

Use automatic buffer, and format_to_n

AlanGriffiths · 2026-03-04T11:29:04Z

src/server/frontend_xwayland/xwayland_spawner.cpp

+constexpr std::string_view x11_lock_fmt = "/tmp/.X{}-lock";
+constexpr std::string_view const x11_socket_fmt = "/tmp/.X11-unix/X{}";

 // TODO this can be written with more modern c++


Can we now remove the TODO?

@AlanGriffiths - Is there still something left to be addressed ?

Copilot

Pull request overview

This PR modernizes string formatting across Mir by replacing fixed-size snprintf(buffer, ...) usage with std::format, aligning with issue #4660 and reducing manual buffer management in multiple components (tests, examples, server logging).

Changes:

Replace multiple snprintf()-built paths/messages with std::format() strings.
Update call sites that require C strings (printf, popen, open, unlink, etc.) to use .c_str().
Refactor a few call sites to keep formatted strings alive where a const char* is stored/passed later.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/performance-tests/test_glmark2-es2.cpp	Switch test log filenames to `std::format`, adjust `printf()` usage, keep host log filename alive via `std::string`.
tests/performance-tests/test_compositor.cpp	Switch server log filename to `std::format`.
tests/mir_test_framework/test_wlcs_display_server.cpp	Switch WAYLAND_DISPLAY value construction to `std::format`.
src/server/report/logging/compositor_report.cpp	Switch compositor report log messages to `std::format`.
src/server/frontend_xwayland/xwayland_spawner.cpp	Switch X11 lock/socket path construction to `std::format` / `format_to_n`.
src/core/logging/logger.cpp	Switch timestamp formatting to chrono + `std::format`.
src/common/logging/input_timestamp.cpp	Switch input timestamp formatting to `std::format`.
examples/mir_demo_server/server_example_test_client.cpp	Switch client-status messages to `std::format`.
examples/mir_demo_server/server_example.cpp	Switch apport command construction to `std::format`.

tests/performance-tests/test_compositor.cpp

tests/mir_test_framework/test_wlcs_display_server.cpp

src/server/report/logging/compositor_report.cpp

Copilot · 2026-03-04T11:49:32Z

src/server/frontend_xwayland/xwayland_spawner.cpp

+constexpr std::string_view x11_lock_fmt = "/tmp/.X{}-lock";
+constexpr std::string_view const x11_socket_fmt = "/tmp/.X11-unix/X{}";

 // TODO this can be written with more modern c++
 int create_lockfile(int xdisplay)
 {
-    char lockfile[256];
+    auto const lockfile = std::format(x11_lock_fmt, xdisplay);

-    snprintf(lockfile, sizeof lockfile, x11_lock_fmt, xdisplay);
-
-    mir::Fd const fd{open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444)};
+    mir::Fd const fd{open(lockfile.c_str(), O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444)};


std::format()'s first parameter is a std::format_string<...> (intended for string literals) and generally won't accept a std::string_view variable like x11_lock_fmt. This is likely a compile error with standard library implementations. Use a std::format_string<int> constant (constructed from the literal) or inline the string literal in the std::format() call.

While it is probably better to use std::format_string<> constants over std::string_view I don't think this will fail to compile.

Each of these format strings are is used twice.
Once with: int xdisplay, once with int xdisplay.
So we cannot have one flexible definition like std::format_string<int | int&>'

Copilot · 2026-03-04T11:49:32Z

src/server/frontend_xwayland/xwayland_spawner.cpp

+    auto eoln = std::format_to_n(addr.sun_path + 1, sizeof(addr.sun_path) - 2,
+        x11_socket_fmt, xdisplay).out;
+    *eoln = '\0';
+    path_size = eoln - (addr.sun_path + 1);
    create_socket(result, &addr, path_size);

-    path_size = snprintf(addr.sun_path, sizeof(addr.sun_path), x11_socket_fmt, xdisplay);
+    eoln = std::format_to_n(addr.sun_path, sizeof(addr.sun_path) - 1,
+        x11_socket_fmt, xdisplay).out;
+    *eoln = '\0';
+    path_size = eoln - addr.sun_path;


std::format_to_n() also expects a compile-time checked std::format_string<...>; passing x11_socket_fmt (a std::string_view) is likely to fail to compile. Consider making x11_socket_fmt a std::format_string<int> (or inline the literal) so format_to_n() can be instantiated.

Copilot · 2026-03-04T11:49:32Z

src/core/logging/logger.cpp

+    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::string fmt_now = std::format("{:%F %T}", local);


This timestamp construction can throw (e.g., std::chrono::current_zone()/zoned_time when the tzdb is missing/corrupt, or std::format on allocation failure). Since format_message() is used by the logging path without exception handling, an exception here can terminate the process. Please make this path non-throwing (e.g., wrap in try/catch and fall back to a simpler timestamp such as UTC, or revert to the previous clock_gettime/strftime approach).

Suggested change

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::string fmt_now = std::format("{:%F %T}", local);

std::string fmt_now;

try

{

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};

fmt_now = std::format("{:%F %T}", local);

}

catch (...)

{

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

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

char buf[32]{};

std::tm tm{};

if (gmtime_r(&now_time_t, &tm) != nullptr &&

std::strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", &tm))

{

fmt_now = buf;

}

else

{

fmt_now = "unknown-time";

}

}

done as suggested.

Copilot · 2026-03-04T11:49:33Z

examples/mir_demo_server/server_example_test_client.cpp

-        char buffer[sizeof format];
-        snprintf(buffer, sizeof buffer, format, signal);
-        ml::log(ml::Severity::informational, buffer, component);
+        auto const msg = std::format(" Client terminated by signal {}", signal);


The log message has a leading space (" Client..."), which looks unintentional and changes the output compared to the previous message text. Remove the leading whitespace.

Suggested change

auto const msg = std::format(" Client terminated by signal {}", signal);

auto const msg = std::format("Client terminated by signal {}", signal);

AlanGriffiths · 2026-03-05T12:21:26Z

src/core/logging/logger.cpp

+        char buf[32]{};
+        std::tm tm{};
+        if (gmtime_r(&now_time_t, &tm) != nullptr &&
+            std::strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", &tm))


No milliseconds?

microseconds added in recent commit

robert-ancell · 2026-03-05T21:04:23Z

src/common/logging/input_timestamp.cpp

+        when_ns, milliseconds, sub_milliseconds, sign_suffix);

-    return std::string(str);
+    return fmt_now;


No need for fmt_now - just return directly.

robert-ancell · 2026-03-05T21:09:56Z

tests/performance-tests/test_glmark2-es2.cpp

-                 "/tmp/%s_%s_host.log",
+        s_host_output_filename = std::format("/tmp/{}_{}_host.log",
                 test_info->test_case_name(), test_info->name());
+        host_output_filename = s_host_output_filename.c_str();


This doesn't seem necessary anymore - can't we just use s_host_output_filename.c_str() in all cases below?

robert-ancell · 2026-03-05T21:10:55Z

src/core/logging/logger.cpp

-        << std::endl;
-
+    out << std::format("[{}] {}{}: {}\n",
+        fmt_now, lut[static_cast<int>(severity)], component, message);


By removing the use of std::endl we are no longer flushing the output stream - is that intentional?

robert-ancell

There's a mix of simple and more complicated changes in here. As these changes are independent of each other I suggest you split this into a number of PRs. Some changes can then land quickly and others that require more thought can take longer.

yotam-medini · 2026-03-05T22:41:13Z

@robert-ancell @RAOF @AlanGriffiths
For sure, I want to address all robert-ancell's remarks.
But in light of his last of his split-PR suggestion,
let's have some consensus from all current reviewers - if to split, and how.

This is my first PR for MIR. I would like to proceed efficiently

robert-ancell · 2026-03-06T00:23:34Z

The easiest way to split is to grab a simple case like in examples/mir_demo_server/server_example.cpp and make a new PR for that. Once that lands then merge this branch with master and it will get smaller. Keep iterating until this PR becomes smaller and thus easier to review.

yotam-medini · 2026-03-06T09:01:26Z

Followed Robert Ancell advice, I converted this PR to draft for now.
Please see #4750

Closes nothing.  Related: #4660  Related to PR #4736  std::format instead of snprintf in server_example.cpp ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

Closes nothing.  Related: #4660  ## What's new? Related to PR #4736  std::format instead of snprintf in TestWlcsDisplayServer(...) ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

Closes nothing.  Related: #4660  ## What's new? Related to PR #4736 std::format instead of snprintf in test_compositor.cpp  ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

Closes nothing.  Related: #4660  ## What's new? Related to PR #4736 std::format instead of snprintf in test_glmark2-es2.cpp  ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

) Closes nothing  Related: to PR #4736 std::format instead of snprintf in server_example_test_client.cpp  ## What's new?  ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

Closes nothing  Related: #4660  Related to PR #4736 ## What's new? std::format instead of snprintf in input_timestamp.cpp  ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

Closes nothing  Related: #4660  Related to #4736 ## What's new? std::format instead of snprintf in compositor_report.cpp  ## How to test  ## Checklist - [ ] Tests added and pass - [ ] Adequate documentation added - [ ] (optional) Added Screenshots or videos

task-4660 std::format instead of snprintf(buffer...) in 9 files

31fccf8

yotam-medini requested a review from a team as a code owner March 2, 2026 12:42

RAOF reviewed Mar 4, 2026

View reviewed changes

pre-commit: apply automatic fixes

c5f5afb

AlanGriffiths requested changes Mar 4, 2026

View reviewed changes

AlanGriffiths requested a review from Copilot March 4, 2026 11:42

Copilot started reviewing on behalf of AlanGriffiths March 4, 2026 11:43 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

pr canonical#4736 - code-review v01

f5ded3c

AlanGriffiths reviewed Mar 5, 2026

View reviewed changes

output microseconds in ml::format_message for the exception-catch case

9e46c6d

robert-ancell reviewed Mar 5, 2026

View reviewed changes

robert-ancell requested changes Mar 5, 2026

View reviewed changes

yotam-medini marked this pull request as draft March 6, 2026 09:00

yotam-medini mentioned this pull request Mar 6, 2026

std::format in server_example.cpp (see issue #4660) #4750

Merged

3 tasks

yotam-medini mentioned this pull request Mar 7, 2026

std::format in test_wlcs_display_server.cpp (see issue #4660)) #4752

Merged

3 tasks

yotam-medini mentioned this pull request Mar 8, 2026

std::format in test_compositor.cpp #4753

Merged

3 tasks

yotam-medini mentioned this pull request Mar 9, 2026

std::format in test_glmark2.cpp (see issue #4660) #4755

Merged

3 tasks

yotam-medini mentioned this pull request Mar 10, 2026

std::format instead of snprintf in server_example_test_client.cpp #4758

Merged

3 tasks

yotam-medini mentioned this pull request Mar 10, 2026

std::format instead of snprintf in input_timestamp.cpp #4759

Merged

3 tasks

yotam-medini mentioned this pull request Mar 11, 2026

std::format instead of snprintf in compositor_report.cpp #4760

Merged

3 tasks

yotam-medini mentioned this pull request Mar 11, 2026

std::format instead of snprintf in logger.cpp #4767

Open

3 tasks

	auto const msg = std::format(" Client terminated by signal {}", signal);
	auto const msg = std::format("Client terminated by signal {}", signal);

Conversation

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

Uh oh!

What's new?

How to test

Checklist

Uh oh!

RAOF Mar 4, 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 Mar 4, 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!

RAOF left a comment

Choose a reason for hiding this comment

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!

yotam-medini Mar 4, 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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

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!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

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

yotam-medini commented Mar 2, 2026 •

edited

Loading

RAOF Mar 4, 2026 •

edited

Loading

yotam-medini Mar 4, 2026 •

edited

Loading

yotam-medini Mar 4, 2026 •

edited

Loading

yotam-medini commented Mar 6, 2026 •

edited

Loading