wgtmac commented on code in PR #725:
URL: https://github.com/apache/iceberg-cpp/pull/725#discussion_r3507615943
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +375,182 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// 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
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 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
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 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
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 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
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (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.
+#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.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
+ do {
\
+ if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {
\
+ const auto& _ib_logger = ::iceberg::internal::CurrentLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(level_)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, (level_),
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
+ } catch (const std::exception&) {
\
+ ::iceberg::internal::EmitFormatError(*_ib_logger, (level_),
\
+
::std::source_location::current()); \
+ }
\
+ }
\
+ }
\
+ } while (0)
+
+#define ICEBERG_LOG_TRACE(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kTrace, __VA_ARGS__)
+#define ICEBERG_LOG_DEBUG(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kDebug, __VA_ARGS__)
+#define ICEBERG_LOG_INFO(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kInfo, __VA_ARGS__)
+#define ICEBERG_LOG_WARN(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kWarn, __VA_ARGS__)
+#define ICEBERG_LOG_ERROR(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kError, __VA_ARGS__)
+#define ICEBERG_LOG_CRITICAL(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kCritical, __VA_ARGS__)
+
+// FATAL: emit if enabled (never compile-stripped), then ALWAYS flush + abort.
+// Acquires the effective (scoped-or-default) logger ONCE -- honoring any
active
+// ScopedLogger binding -- and uses that same instance for emit and flush, so a
+// concurrent SetDefaultLogger cannot flush a different logger than it emitted
to.
+#define ICEBERG_LOG_FATAL(FMT_, ...)
\
+ do {
\
+ auto _ib_logger = ::iceberg::GetCurrentLogger();
\
Review Comment:
Can we add a small death/regression test for this path under `ScopedLogger`?
That would lock in the switch from `GetDefaultLogger()` to `GetCurrentLogger()`
for fixed `ICEBERG_LOG_FATAL`.
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -64,7 +64,9 @@ function(add_iceberg_test test_name)
endif()
if(MSVC_TOOLCHAIN)
- target_compile_options(${test_name} PRIVATE /bigobj)
+ # /Zc:preprocessor: conforming preprocessor for the __VA_OPT__ in the
logging
+ # macros (MSVC's traditional preprocessor rejects it).
+ target_compile_options(${test_name} PRIVATE /bigobj /Zc:preprocessor)
Review Comment:
Meson adds `/Zc:preprocessor` project-wide, but CMake only adds it to tests.
Since `logger.h` is public and `logger.cc` includes it, the library targets
(and ideally consumers) also need the conforming preprocessor option on MSVC.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +375,182 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// 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
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 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
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 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
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 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
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (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
Review Comment:
`if constexpr` does not remove this at preprocessing time. Even when the
branch is discarded, the log statement still has to be
syntactically/semantically valid, so bad format strings or non-formattable args
may still fail to compile. Maybe soften “removed entirely” here.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +375,182 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// 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
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 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
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 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
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 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
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (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.
+#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.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
+ do {
\
+ if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {
\
+ const auto& _ib_logger = ::iceberg::internal::CurrentLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(level_)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, (level_),
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
+ } catch (const std::exception&) {
\
Review Comment:
This no longer fully matches the “logging never throws” guarantee.
`std::format` can call user-defined formatters, and those can throw
non-`std::exception` types; with this catch they escape (or hit `noexcept` in
the function API). Either keep the catch-all or narrow the guarantee.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +375,182 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// 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
Review Comment:
The example output is missing the opening `[` before the source location.
`CerrLogger` formats it as `[file:line]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
