From ccb1cbb7e42bc7f2067f4aa9f66a5ff9c90c94ac Mon Sep 17 00:00:00 2001
From: Ben Deane <ben@elbeno.com>
Date: Wed, 4 Dec 2024 13:46:49 -0700
Subject: [PATCH] :art: Improve CIB_FATAL

Problem:
- `CIB_FATAL` formats the string it passes to `log`, but passes the bare
  arguments to `stdx::panic`.

Solution:
- Pass the formatted string to `stdx::panic`.

Note:
- There is a small improvement here to `to_message` also.
---
 include/log/catalog/mipi_encoder.hpp | 20 +++++------------
 include/log/log.hpp                  | 14 +++++++++---
 test/log/log.cpp                     | 33 +++++++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/include/log/catalog/mipi_encoder.hpp b/include/log/catalog/mipi_encoder.hpp
index 4190c592..ffea4d82 100644
--- a/include/log/catalog/mipi_encoder.hpp
+++ b/include/log/catalog/mipi_encoder.hpp
@@ -18,24 +18,14 @@
 
 namespace logging::mipi {
 namespace detail {
-template <logging::level L, typename S, typename T>
+template <logging::level L, typename S, typename... Args>
 constexpr auto to_message() {
     constexpr auto s = S::value;
     using char_t = typename std::remove_cv_t<decltype(s)>::value_type;
-    return [&]<template <typename...> typename Tuple, typename... Args,
-               std::size_t... Is>(Tuple<Args...> const &,
-                                  std::integer_sequence<std::size_t, Is...>) {
+    return [&]<std::size_t... Is>(std::integer_sequence<std::size_t, Is...>) {
         return sc::message<
             L, sc::undefined<sc::args<Args...>, char_t, s[Is]...>>{};
-    }(T{}, std::make_integer_sequence<std::size_t, std::size(s)>{});
-}
-
-template <logging::level L, typename Msg> constexpr auto to_message(Msg msg) {
-    if constexpr (requires { msg.args; }) {
-        return to_message<L, decltype(msg.str), decltype(msg.args)>();
-    } else {
-        return to_message<L, Msg, stdx::tuple<>>();
-    }
+    }(std::make_integer_sequence<std::size_t, std::size(s)>{});
 }
 
 template <typename S> constexpr auto to_module() {
@@ -122,8 +112,8 @@ template <typename TDestinations> struct log_handler {
 
     template <logging::level Level, typename ModuleId, typename Msg>
     ALWAYS_INLINE auto log_msg(Msg msg) -> void {
-        msg.apply([&]<typename StringType>(StringType, auto... args) {
-            using Message = decltype(detail::to_message<Level>(msg));
+        msg.apply([&]<typename S, typename... Args>(S, Args... args) {
+            using Message = decltype(detail::to_message<Level, S, Args...>());
             using Module = decltype(detail::to_module<ModuleId>());
             dispatch_message<Level>(catalog<Message>(), module<Module>(),
                                     static_cast<std::uint32_t>(args)...);
diff --git a/include/log/log.hpp b/include/log/log.hpp
index 35a0eb9f..3933d063 100644
--- a/include/log/log.hpp
+++ b/include/log/log.hpp
@@ -92,9 +92,17 @@ using cib_log_module_id_t = typename logging::module_id_t<"default">::type;
     CIB_LOG(logging::default_flavor_t, logging::level::WARN, __VA_ARGS__)
 #define CIB_ERROR(...)                                                         \
     CIB_LOG(logging::default_flavor_t, logging::level::ERROR, __VA_ARGS__)
-#define CIB_FATAL(...)                                                         \
-    (CIB_LOG(logging::default_flavor_t, logging::level::FATAL, __VA_ARGS__),   \
-     STDX_PANIC(__VA_ARGS__))
+
+#define CIB_FATAL(MSG, ...)                                                    \
+    [] {                                                                       \
+        constexpr auto str = sc::format(MSG##_sc __VA_OPT__(, ) __VA_ARGS__);  \
+        logging::log<logging::default_flavor_t, logging::level::FATAL,         \
+                     cib_log_module_id_t>(__FILE__, __LINE__, str);            \
+        str.apply([]<typename S, typename... Args>(S s, Args... args) {        \
+            constexpr auto cts = stdx::ct_string_from_type(s);                 \
+            stdx::panic<cts>(args...);                                         \
+        });                                                                    \
+    }()
 
 #define CIB_ASSERT(expr)                                                       \
     ((expr) ? void(0) : CIB_FATAL("Assertion failure: " #expr))
diff --git a/test/log/log.cpp b/test/log/log.cpp
index dab38628..e5406da2 100644
--- a/test/log/log.cpp
+++ b/test/log/log.cpp
@@ -1,3 +1,4 @@
+#include <log/fmt/logger.hpp>
 #include <log/log.hpp>
 
 #include <stdx/ct_string.hpp>
@@ -5,6 +6,10 @@
 
 #include <catch2/catch_test_macros.hpp>
 
+#include <iterator>
+#include <string>
+#include <string_view>
+
 namespace {
 bool panicked{};
 
@@ -15,12 +20,38 @@ struct injected_handler {
         panicked = true;
     }
 };
+
+std::string buffer{};
 } // namespace
 
+template <>
+inline auto logging::config<> =
+    logging::fmt::config{std::back_inserter(buffer)};
+
 template <> inline auto stdx::panic_handler<> = injected_handler{};
 
-TEST_CASE("FATAL calls compile-time panic", "[log]") {
+TEST_CASE("CIB_FATAL logs the string", "[log]") {
+    CIB_FATAL("Hello");
+    CAPTURE(buffer);
+    CHECK(buffer.substr(buffer.size() - std::size("Hello")) == "Hello\n");
+}
+
+TEST_CASE("CIB_FATAL respects the log module", "[log]") {
+    CIB_LOG_MODULE("test");
+    CIB_FATAL("Hello");
+    CAPTURE(buffer);
+    CHECK(buffer.substr(buffer.size() - std::size("FATAL [test]: Hello")) ==
+          "FATAL [test]: Hello\n");
+}
+
+TEST_CASE("CIB_FATAL calls compile-time panic", "[log]") {
     panicked = false;
     CIB_FATAL("Hello");
     CHECK(panicked);
 }
+
+TEST_CASE("CIB_FATAL pre-formats arguments passed to panic", "[log]") {
+    panicked = false;
+    CIB_FATAL("{}", "Hello"_sc);
+    CHECK(panicked);
+}