fix(logging): address #725 review (catch-all, /Zc:preprocessor, docs, FATAL test)

- Revert the 6 catch sites to catch(...) so the noexcept "logging never throws"
  guarantee holds even when a user-defined std::formatter throws a non-std type
  (catch(const std::exception&) would let it reach std::terminate).
- Apply /Zc:preprocessor to the iceberg library targets PUBLICly on MSVC, not
  only to tests: logger.h is public and uses __VA_OPT__, so the library build and
  consumers need the conforming preprocessor too.
- Soften the compile-time-floor doc: `if constexpr` discards the emit but the
  statement must still be well-formed (bad format string is still a compile error).
- Add the missing opening '[' to the logger.h example output lines to match
  CerrLogger's [file:line] layout.
- Add MacrosDeathTest.FatalRoutesThroughScopedLogger locking in that
  ICEBERG_LOG_FATAL routes through the active ScopedLogger (GetCurrentLogger).

Co-authored-by: Isaac

Author: kamcheungting-db

src/iceberg/CMakeLists.txt

@@ -186,6 +186,18 @@ add_iceberg_lib(iceberg
                 OUTPUTS
                 ICEBERG_LIBRARIES)
 
+# The logging macros in the public logger.h use __VA_OPT__, which MSVC's
+# traditional preprocessor rejects. Require the conforming preprocessor PUBLICly
+# so both these library targets (logger.cc includes logger.h) and downstream
+# consumers of the installed header get it on MSVC.
+if(MSVC_TOOLCHAIN)
+  foreach(_iceberg_lib iceberg_shared iceberg_static)
+    if(TARGET ${_iceberg_lib})
+      target_compile_options(${_iceberg_lib} PUBLIC /Zc:preprocessor)
+    endif()
+  endforeach()
+endif()
+
 set(ICEBERG_DATA_SOURCES
     data/data_writer.cc
     data/delete_filter.cc

src/iceberg/logging/logger.h

@@ -28,7 +28,6 @@
 
 #include <concepts>
 #include <cstdlib>
-#include <exception>
 #include <format>
 #include <memory>
 #include <source_location>
@@ -340,10 +339,10 @@ void FormatAndEmit(Logger& logger, LogLevel level, const std::source_location& l
   if (!logger.ShouldLog(level)) return;
   try {
     Emit(logger, level, loc, std::format(fmt, std::forward<Args>(args)...));
-  } catch (const std::exception&) {
-    // The only throws here are std::format_error / std::bad_alloc -- same recovery
-    // for both (drop, emit a fixed marker). Catch std::exception, not (...), so a
-    // non-std unwind like abi::__forced_unwind (thread cancellation) propagates.
+  } catch (...) {
+    // Catch-all upholds the noexcept "logging never throws" guarantee: a
+    // user-defined std::formatter may throw a non-std::exception type, and this
+    // function is noexcept, so anything escaping here would call std::terminate.
     EmitFormatError(logger, level, loc);
   }
 }
@@ -385,21 +384,22 @@ void Log(Logger& logger, LogLevel level,
 // site and, for the default CerrLogger, the line it produces.
 //
 //   ICEBERG_LOG_TRACE("entering scan for {}", table);
-//     2026-06-16T10:59:41.186Z trace [12345] table_scan.cc:88] entering scan for db.t
+//     2026-06-16T10:59:41.186Z trace [12345] [table_scan.cc:88] entering scan for db.t
 //   ICEBERG_LOG_DEBUG("cache miss key={}", key);
-//     2026-06-16T10:59:41.186Z debug [12345] cache.cc:42] cache miss key=manifest-7
+//     2026-06-16T10:59:41.186Z debug [12345] [cache.cc:42] cache miss key=manifest-7
 //   ICEBERG_LOG_INFO("loaded {} manifests in {} ms", n, ms);
-//     2026-06-16T10:59:41.186Z info [12345] table_scan.cc:91] loaded 5 manifests in 12 ms
+//     2026-06-16T10:59:41.186Z info [12345] [table_scan.cc:91] loaded 5 manifests in 12
+//     ms
 //   ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
-//     2026-06-16T10:59:41.186Z warn [12345] io.cc:51] retry 2 after timeout
+//     2026-06-16T10:59:41.186Z warn [12345] [io.cc:51] retry 2 after timeout
 //   ICEBERG_LOG_ERROR("commit failed: {}", status);
-//     2026-06-16T10:59:41.186Z error [12345] txn.cc:77] commit failed: conflict
+//     2026-06-16T10:59:41.186Z error [12345] [txn.cc:77] commit failed: conflict
 //   ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
-//     2026-06-16T10:59:41.186Z critical [12345] meta.cc:30] metadata unreadable at
+//     2026-06-16T10:59:41.186Z critical [12345] [meta.cc:30] metadata unreadable at
 //     s3://b/m.json
 //   ICEBERG_LOG_FATAL("unrecoverable: {}", reason);   // emits, flushes, then
 //   std::abort()
-//     2026-06-16T10:59:41.186Z fatal [12345] boot.cc:19] unrecoverable: bad config
+//     2026-06-16T10:59:41.186Z fatal [12345] [boot.cc:19] unrecoverable: bad config
 //
 // Less common forms:
 //   ICEBERG_LOG(level, "level chosen at runtime: {}", x);     // runtime severity
@@ -411,19 +411,22 @@ void Log(Logger& logger, LogLevel level,
 // (ICEBERG_LOG_INFO("done")).
 // ---------------------------------------------------------------------------
 
-/// \brief Compile-time severity floor: statements below this level are removed
-/// entirely from the build (their format call sites and source_location literals
-/// are never emitted). Defaults to keeping everything. ICEBERG_LOG_FATAL is never
-/// gated by this floor -- its abort is always compiled in.
+/// \brief Compile-time severity floor: statements below this level are discarded
+/// via `if constexpr`, so no emit code runs and no format call / source_location
+/// is generated for them (the compiler is free to optimize the dead branch away).
+/// The statement must still be well-formed -- a bad format string or a
+/// non-formattable argument is a compile error even when the branch is discarded.
+/// Defaults to keeping everything. ICEBERG_LOG_FATAL is never gated by this floor
+/// -- its abort is always compiled in.
 #ifndef ICEBERG_LOG_ACTIVE_LEVEL
 #  define ICEBERG_LOG_ACTIVE_LEVEL ::iceberg::LogLevel::kTrace
 #endif
 
 // Internal: fixed-severity emit with compile-time floor then the authoritative
 // Logger::ShouldLog (the single source of truth for runtime filtering), with
-// formatting only on the taken path, never throwing. The catch handles
-// std::exception (std::format_error / std::bad_alloc) -- not (...) -- so a non-std
-// unwind such as abi::__forced_unwind (thread cancellation) still propagates.
+// formatting only on the taken path, never throwing. The catch-all upholds the
+// "logging never throws" guarantee even when a user-defined std::formatter throws
+// a non-std::exception type (a std::format_error/bad_alloc route to EmitFormatError).
 #define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)                                      \
   do {                                                                               \
     if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {                            \
@@ -433,7 +436,7 @@ void Log(Logger& logger, LogLevel level,
           ::iceberg::internal::Emit(*_ib_logger, (level_),                           \
                                     ::std::source_location::current(),               \
                                     ::std::format(FMT_ __VA_OPT__(, ) __VA_ARGS__)); \
-        } catch (const std::exception&) {                                            \
+        } catch (...) {                                                              \
           ::iceberg::internal::EmitFormatError(*_ib_logger, (level_),                \
                                                ::std::source_location::current());   \
         }                                                                            \
@@ -466,7 +469,7 @@ void Log(Logger& logger, LogLevel level,
         ::iceberg::internal::Emit(*_ib_logger, ::iceberg::LogLevel::kFatal,            \
                                   ::std::source_location::current(),                   \
                                   ::std::format(FMT_ __VA_OPT__(, ) __VA_ARGS__));     \
-      } catch (const std::exception&) {                                                \
+      } catch (...) {                                                                  \
         ::iceberg::internal::EmitFormatError(*_ib_logger, ::iceberg::LogLevel::kFatal, \
                                              ::std::source_location::current());       \
       }                                                                                \
@@ -487,7 +490,7 @@ void Log(Logger& logger, LogLevel level,
         ::iceberg::internal::Emit(*_ib_logger, _ib_lvl,                            \
                                   ::std::source_location::current(),               \
                                   ::std::format(FMT_ __VA_OPT__(, ) __VA_ARGS__)); \
-      } catch (const std::exception&) {                                            \
+      } catch (...) {                                                              \
         ::iceberg::internal::EmitFormatError(*_ib_logger, _ib_lvl,                 \
                                              ::std::source_location::current());   \
       }                                                                            \
@@ -509,7 +512,7 @@ void Log(Logger& logger, LogLevel level,
         ::iceberg::internal::Emit(_ib_logger, _ib_lvl,                             \
                                   ::std::source_location::current(),               \
                                   ::std::format(FMT_ __VA_OPT__(, ) __VA_ARGS__)); \
-      } catch (const std::exception&) {                                            \
+      } catch (...) {                                                              \
         ::iceberg::internal::EmitFormatError(_ib_logger, _ib_lvl,                  \
                                              ::std::source_location::current());   \
       }                                                                            \
@@ -531,7 +534,7 @@ void Log(Logger& logger, LogLevel level,
         ::iceberg::internal::Emit(                                               \
             *_ib_logger, _ib_lvl, ::std::source_location::current(),             \
             ::iceberg::internal::VFormat((FMT_)__VA_OPT__(, ) __VA_ARGS__));     \
-      } catch (const std::exception&) {                                          \
+      } catch (...) {                                                            \
         ::iceberg::internal::EmitFormatError(*_ib_logger, _ib_lvl,               \
                                              ::std::source_location::current()); \
       }                                                                          \

src/iceberg/test/macros_test.cc

@@ -147,4 +147,18 @@ TEST(MacrosDeathTest, LogToFatalEmitsThenAborts) {
       "tofatal 2");
 }
 
+// Regression guard: ICEBERG_LOG_FATAL must route through the active ScopedLogger
+// binding (GetCurrentLogger), not the process default (GetDefaultLogger). A scoped
+// CerrLogger emits the record before the abort; the message must reach that sink.
+TEST(MacrosDeathTest, FatalRoutesThroughScopedLogger) {
+  EXPECT_DEATH(
+      {
+        SetDefaultLevel(LogLevel::kOff);  // default gate off -> only the scope emits
+        auto scoped = std::make_shared<CerrLogger>(LogLevel::kTrace);
+        ScopedLogger bind(scoped);
+        ICEBERG_LOG_FATAL("scopedfatal {}", 9);
+      },
+      "scopedfatal 9");
+}
+
 }  // namespace iceberg