Logging fmt syntax

examples/mir_demo_server/server_example_test_client.cpp

@@ -143,11 +143,14 @@ void me::TestClientRunner::operator()(mir::Server& server)
                 setenv("SDL_VIDEODRIVER", "wayland", true);         // configure SDL to use Wayland
 
                 auto const client = options1->get<std::string>(test_client_opt);
-                log(logging::Severity::informational, "mir::examples", "Starting test client: %s", client.c_str());
+                log(logging::Severity::informational, "mir::examples", "Starting test client: {}", client);
                 execlp(client.c_str(), client.c_str(), static_cast<char const*>(nullptr));
                 // If execl() returns then something is badly wrong
-                log(logging::Severity::critical, "mir::examples",
-                    "Failed to execute client (%s) error: %s", client.c_str(), mir::errno_to_cstr(errno));
+                log(logging::Severity::critical,
+                    "mir::examples",
+                    "Failed to execute client ({}) error: {}",
+                    client,
+                    mir::errno_to_cstr(errno));
                 abort();
             }
             else if (pid > 0)

include/core/mir/log.h

@@ -21,17 +21,12 @@
 
 #include <mir/logging/logger.h>  // for Severity
 #include <string>
-#include <cstdarg>
+#include <format>
+#include <utility>
 #include <exception>
 
 namespace mir
 {
-[[gnu::format (printf, 3, 0)]]
-void logv(logging::Severity sev, const char *component,
-          char const* fmt, va_list va);
-[[gnu::format (printf, 3, 4)]]
-void log(logging::Severity sev, const char *component,
-         char const* fmt, ...);
 void log(logging::Severity sev, const char *component,
          std::string const& message);
 void log(
@@ -40,6 +35,13 @@ void log(
     std::exception_ptr const& exception,
     std::string const& message);
 
+template <typename... Args>
+inline void log(
+    logging::Severity sev, std::string_view const component, std::format_string<Args...> fmt, Args&&... args)
+{
+    log(sev, component.data(), std::format(fmt, std::forward<Args>(args)...));
+}
+
 /// Log a security event according to the OWASP specification
 ///
 /// \param severity The severity of the event
@@ -67,34 +69,22 @@ inline void log_info(std::string const& message)
                MIR_LOG_COMPONENT, message);
 }
 
-[[gnu::format (printf, 1, 2)]]
-inline void log_info(char const* fmt, ...)
+template<typename... Args>
+inline void log_info(std::format_string<Args...> fmt, Args&&... args)
 {
-    va_list va;
-    va_start(va, fmt);
-    ::mir::logv(::mir::logging::Severity::informational,
-                MIR_LOG_COMPONENT, fmt, va);
-    va_end(va);
+    ::mir::log(::mir::logging::Severity::informational, MIR_LOG_COMPONENT, fmt, std::forward<Args>(args)...);
 }
 
-[[gnu::format (printf, 1, 2)]]
-inline void log_error(char const* fmt, ...)
+template<typename... Args>
+inline void log_error(std::format_string<Args...> fmt, Args&&... args)
 {
-    va_list va;
-    va_start(va, fmt);
-    ::mir::logv(::mir::logging::Severity::error,
-                MIR_LOG_COMPONENT, fmt, va);
-    va_end(va);
+    ::mir::log(::mir::logging::Severity::error, MIR_LOG_COMPONENT, fmt, std::forward<Args>(args)...);
 }
 
-[[gnu::format (printf, 1, 2)]]
-inline void log_debug(char const* fmt, ...)
+template<typename... Args>
+inline void log_debug(std::format_string<Args...> fmt, Args&&... args)
 {
-    va_list va;
-    va_start(va, fmt);
-    ::mir::logv(::mir::logging::Severity::debug,
-               MIR_LOG_COMPONENT, fmt, va);
-    va_end(va);
+    ::mir::log(::mir::logging::Severity::debug, MIR_LOG_COMPONENT, fmt, std::forward<Args>(args)...);
 }
 
 inline void log_critical(std::string const& message)
@@ -103,14 +93,10 @@ inline void log_critical(std::string const& message)
                MIR_LOG_COMPONENT, message);
 }
 
-[[gnu::format(printf, 1, 2)]]
-inline void log_critical(char const* fmt, ...)
+template<typename... Args>
+inline void log_critical(std::format_string<Args...> fmt, Args&&... args)
 {
-    va_list va;
-    va_start(va, fmt);
-    ::mir::logv(::mir::logging::Severity::critical,
-               MIR_LOG_COMPONENT, fmt, va);
-    va_end(va);
+    ::mir::log(::mir::logging::Severity::critical, MIR_LOG_COMPONENT, fmt, std::forward<Args>(args)...);
 }
 
 inline void log_error(std::string const& message)
@@ -125,14 +111,10 @@ inline void log_warning(std::string const& message)
                MIR_LOG_COMPONENT, message);
 }
 
-[[gnu::format(printf, 1, 2)]]
-inline void log_warning(char const* fmt, ...)
+template<typename... Args>
+inline void log_warning(std::format_string<Args...> fmt, Args&&... args)
 {
-    va_list va;
-    va_start(va, fmt);
-    ::mir::logv(::mir::logging::Severity::warning,
-               MIR_LOG_COMPONENT, fmt, va);
-    va_end(va);
+    ::mir::log(::mir::logging::Severity::warning, MIR_LOG_COMPONENT, fmt, std::forward<Args>(args)...);
 }
 } // (nested anonymous) namespace
 #endif

include/core/mir/logging/logger.h

@@ -17,6 +17,7 @@
 #ifndef MIR_LOGGING_LOGGER_H_
 #define MIR_LOGGING_LOGGER_H_
 
+#include <format>
 #include <memory>
 #include <string>
 #include <iosfwd>
@@ -44,15 +45,11 @@ class Logger
                      const std::string& message,
                      const std::string& component) = 0;
 
-    /*
-     * Those playing at home may wonder why we're saying the 4th argument is the format string,
-     * when it's the 3rd argument in the signature.
-     *
-     * The answer, of course, is that the attribute doesn't know about the implicit
-     * 'this' first parameter of C++!
-     */
-    virtual void log(char const* component, Severity severity, char const* format, ...)
-         __attribute__ ((format (printf, 4, 5)));
+    template <typename... Args>
+    void log(char const* component, Severity severity, std::format_string<Args...> fmt, Args&&... args)
+    {
+        log(severity, std::format(fmt, std::forward<Args>(args)...), component);
+    }
 
 protected:
     Logger() {}

src/common/handle_event_exception.h

@@ -23,6 +23,6 @@
     }                                    \
     catch(std::exception const& e)       \
     {                                    \
-        mir::log_critical("%s", e.what());     \
+        mir::log_critical("{}", e.what());     \
         abort();                         \
     }

src/common/sharedlibrary/shared_library.cpp

@@ -63,7 +63,7 @@ void* mir::SharedLibrary::load_symbol(char const* function_name, char const* ver
 
 #ifdef MIR_DONT_USE_DLVSYM
     // Load the function without checking the version
-    log_debug("Cannot check \"%s\" symbol version is \"%s\": dlvsym() is unavailable", function_name, version);
+    log_debug("Cannot check \"{}\" symbol version is \"{}\": dlvsym() is unavailable", function_name, version);
     return load_symbol(function_name);
 #else
     if (void* result = dlvsym(so, function_name, version))

src/core/log.cpp

@@ -17,7 +17,6 @@
 #include <mir/log.h>
 #include <mir/logging/logger.h>
 #include <chrono>
-#include <cstdio>
 #include <exception>
 #include <format>
 #include <sstream>
@@ -27,36 +26,12 @@
 
 namespace mir {
 
-void logv(logging::Severity sev, char const* component,
-          char const* fmt, va_list va)
-{
-    char message[1024];
-    int max = sizeof(message) - 1;
-    int len = vsnprintf(message, max, fmt, va);
-    if (len > max)
-        len = max;
-    message[len] = '\0';
-
-    // Suboptimal: Constructing a std::string for message/component.
-    logging::log(sev, message, component);
-}
-
-void log(logging::Severity sev, char const* component,
-         char const* fmt, ...)
-{
-    va_list va;
-    va_start(va, fmt);
-    logv(sev, component, fmt, va);
-    va_end(va);
-}
-
 void log(logging::Severity sev, char const* component,
          std::string const& message)
 {
     logging::log(sev, message, component);
 }
 
-
 void log(
     logging::Severity severity,
     char const* component,
@@ -71,20 +46,22 @@ void log(
     {
         // TODO: We can probably format this better by pulling out
         // the boost::errinfo's ourselves.
-        mir::log(
+        logging::log(
             severity,
             component,
-            "%s: %s",
-            message.c_str(),
-            boost::diagnostic_information(err).c_str());
+            std::format(
+                "{}: {}",
+                message.c_str(),
+                boost::diagnostic_information(err).c_str()));
     }
     catch(...)
     {
-        mir::log(
+        logging::log(
             severity,
             component,
-            "%s: unknown exception",
-            message.c_str());
+            std::format(
+                "{}: unknown exception",
+                message));
     }
 }
 

src/core/logging/logger.cpp

@@ -25,19 +25,6 @@
 
 namespace ml = mir::logging;
 
-void ml::Logger::log(char const* component, Severity severity, char const* format, ...)
-{
-    auto const bufsize = 4096;
-    va_list va;
-    va_start(va, format);
-    char message[bufsize];
-    vsnprintf(message, bufsize, format, va);
-    va_end(va);
-
-    // Inefficient, but maintains API: Constructing a std::string for message/component.
-    log(severity, std::string{message}, std::string{component});
-}
-
 namespace
 {
 std::mutex log_mutex;

src/miral/application_switcher.cpp

@@ -79,7 +79,7 @@ struct ToplevelInfoPrinter
 
         if (FT_New_Face(lib, mir::default_font().c_str(), 0, &face))
         {
-            mir::log_error("Failed to load find: %s", mir::default_font().c_str());
+            mir::log_error("Failed to load find: {}", mir::default_font());
             FT_Done_FreeType(lib);
             return;
         }

src/miral/basic_window_manager.cpp

@@ -132,8 +132,8 @@ void miral::BasicWindowManager::remove_session(std::shared_ptr<scene::Session> c
     if (info == app_info.end())
     {
         log_debug(
-            "BasicWindowManager::remove_session() called with unknown or already removed session %s (PID: %d)",
-            session->name().c_str(),
+            "BasicWindowManager::remove_session() called with unknown or already removed session {} (PID: {})",
+            session->name(),
             session->process_id());
         return;
     }
@@ -237,8 +237,8 @@ void miral::BasicWindowManager::remove_surface(
     if (app_info.find(session) == app_info.end())
     {
         log_debug(
-            "BasicWindowManager::remove_surface() called with unknown or already removed session %s (PID: %d)",
-            session->name().c_str(),
+            "BasicWindowManager::remove_surface() called with unknown or already removed session {} (PID: {})",
+            session->name(),
             session->process_id());
         return;
     }
@@ -1773,10 +1773,7 @@ auto miral::BasicWindowManager::surface_known(
         else
             description = "null surface";
 
-        log_debug(
-            "%s requested on %s",
-            action.c_str(),
-            description.c_str());
+        log_debug("{} requested on {}", action, description);
 
         return false;
     }

src/miral/bounce_keys.cpp

@@ -171,8 +171,8 @@ miral::BounceKeys::BounceKeys(live_config::Store& config_store) :
                 else
                 {
                     mir::log_warning(
-                        "Config value %s does not support negative values. Ignoring the supplied value (%d)...",
-                        key.to_string().c_str(),
+                        "Config value {} does not support negative values. Ignoring the supplied value ({})...",
+                        key.to_string(),
                         *val);
                 }
             }

src/miral/config_file.cpp

@@ -105,7 +105,7 @@ Watcher::Watcher(path file, miral::ConfigFile::Loader load_config) :
 {
     if (directory_watch_descriptor.has_value())
     {
-        mir::log_debug("Monitoring %s for configuration changes", (directory.value()/filename).c_str());
+        mir::log_debug("Monitoring {} for configuration changes", (directory.value() / filename).string());
     }
 }
 
@@ -143,7 +143,7 @@ miral::ConfigFile::Self::Self(MirRunner& runner, path file, Mode mode, Loader lo
         if (std::ifstream config_file{filepath})
         {
             load_config(config_file, filepath);
-            mir::log_debug("Loaded %s", filepath.c_str());
+            mir::log_debug("Loaded {}", filepath.string());
             break;
         }
     }
@@ -213,11 +213,11 @@ void Watcher::handler(int) const
                     if (std::ifstream config_file{file})
                     {
                         load_config(config_file, file);
-                        mir::log_debug("(Re)loaded %s", file.c_str());
+                        mir::log_debug("(Re)loaded {}", file.string());
                     }
                     else
                     {
-                        mir::log_debug("Failed to open %s", file.c_str());
+                        mir::log_debug("Failed to open {}", file.string());
                     }
                 }
             }

src/miral/cursor_scale.cpp

@@ -157,8 +157,9 @@ miral::CursorScale::CursorScale(live_config::Store& config_store) : miral::Curso
                 else
                 {
                     mir::log_warning(
-                        "Config value %s does not support negative values. Ignoring the supplied value (%f)...",
-                        key.to_string().c_str(), *val);
+                        "Config value {} does not support negative values. Ignoring the supplied value ({})...",
+                        key.to_string(),
+                        *val);
                 }
             }
             else

src/miral/cursor_theme.cpp

@@ -66,7 +66,7 @@ void miral::CursorTheme::operator()(mir::Server& server) const
                 if (has_default_cursor(*xcursor_loader))
                     return xcursor_loader;
 
-                mir::log_warning("Failed to load cursor theme: %s", theme.c_str());
+                mir::log_warning("Failed to load cursor theme: {}", theme);
 
                 if ((i = j) != std::end(themes)) ++i;
             }

src/miral/hover_click.cpp

@@ -84,8 +84,8 @@ miral::HoverClick::HoverClick(miral::live_config::Store& config_store)
 
                 if (*val < 0)
                     mir::log_warning(
-                        "Config value %s does not support negative values. Ignoring the supplied value (%d)...",
-                        key.to_string().c_str(),
+                        "Config value {} does not support negative values. Ignoring the supplied value ({})...",
+                        key.to_string(),
                         *val);
 
                 ovo(*val);