kamcheungting-db commented on PR #725: URL: https://github.com/apache/iceberg-cpp/pull/725#issuecomment-4989175197
Thanks — #824 was a helpful concrete reference. I've folded the structural direction into this PR: **Adopted from #824** - **Header split:** `logger.h` is now the C++ API only. `ICEBERG_LOG_*` macros moved to a new `log_macros.h`; the bare `LOG_*` aliases moved behind an opt-in `short_log_macros.h` (replacing the `ICEBERG_LOG_SHORT_MACROS` define). Consumers of just the C++ API no longer pull in the macros or the conforming-preprocessor requirement. - **Deduplication:** the five macro bodies now delegate to shared `internal::` helpers (`EmitIfEnabled` + `LogToCurrent` / `LogToCurrentRuntime` / `LogToExplicitRuntime` / `LogFatal`) taking a lazy `[&]()->std::string` message thunk invoked only past `ShouldLog`, so disabled logs still don't evaluate their arguments. `VFormat` moved alongside the macros (only `ICEBERG_LOG_RUNTIME_FMT` uses it). **One deliberate difference:** I kept the `if constexpr` compile-time floor rather than the preprocessor `#if … ((void)0)` gating. With `if constexpr`, a disabled fixed-level log is still parsed and type-checked, so a bad format string / non-formattable arg is caught immediately instead of hiding until the floor is lowered — consistent with your earlier note that the statement must remain well-formed. Happy to switch if you'd prefer the spdlog-style "not even compiled" guarantee. Also addressed the rest of the review in this round: - `ICEBERG_LOG_FATAL` routes through `GetCurrentLogger()` (scoped-aware), with a `FatalRoutesThroughScopedLogger` regression death test. - Reverted to `catch (...)` to keep the noexcept "logging never throws" guarantee for user formatters that throw non-`std::exception` types. - `/Zc:preprocessor` is now applied PUBLIC to the library targets on MSVC (not just tests). - Softened the compile-time-floor doc wording; fixed the example output to `[file:line]`. - **Extensibility:** added a registerable `SetFatalHandler` invoked on the fatal path (after emit+flush, before `abort()`), so a JNI/Python host or crash reporter can flush resources / print a stack trace; the handler receives the source location and formatted message. Death tests cover it. The owned-`std::string` message is unchanged here; the zero-allocation `LogRecord`/reused-buffer path is in #795, and I'll reconcile the thunk with it when that lands. -- 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]
