Copilot commented on code in PR #737: URL: https://github.com/apache/iceberg-cpp/pull/737#discussion_r3619371143
########## src/iceberg/logging/log_macros.h: ########## @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/logging/log_macros.h +/// \brief Iceberg-prefixed logging macros (ICEBERG_LOG_*). +/// +/// Kept out of logger.h so consumers of the C++ logging API (Logger, Log(), +/// ScopedLogger) are not forced to pull in these macros or the conforming +/// preprocessor they require. Include this header to use the macros; include +/// short_log_macros.h for the bare LOG_* aliases. + +#include <cstdlib> +#include <format> +#include <memory> +#include <source_location> +#include <string> +#include <string_view> +#include <utility> + +#include "iceberg/logging/log_level.h" +#include "iceberg/logging/logger.h" + +namespace iceberg::internal { + +/// \brief Runtime (non-literal) format-string helper for ICEBERG_LOG_RUNTIME_FMT. +/// +/// std::format requires a compile-time format string; this routes a runtime +/// string through std::vformat. Args are bound as named lvalues and the +/// arg-store is held in a named variable so it outlives the vformat call +/// (C++23 make_format_args rejects rvalues -- P2905 / LWG3631). +template <typename... Args> +std::string VFormat(std::string_view fmt, Args&&... args) { + auto store = std::make_format_args(args...); + return std::vformat(fmt, store); +} + +/// \brief Gate on \p logger.ShouldLog, then format (via \p make_message) and emit. +/// +/// \p make_message is a callable returning the formatted std::string; it is +/// invoked only after the level passes ShouldLog, so a disabled log never +/// evaluates its format arguments. Never throws: a std::formatter that throws +/// (any type) routes to EmitFormatError, so the noexcept logging contract holds. +template <typename MakeMessage> +void EmitIfEnabled(Logger& logger, LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + if (!logger.ShouldLog(level)) return; + try { + Emit(logger, level, location, std::forward<MakeMessage>(make_message)()); + } catch (...) { + EmitFormatError(logger, level, location); + } +} + +/// \brief Emit to the current (scoped-or-default) logger if enabled. +template <typename MakeMessage> +void LogToCurrent(LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + const std::shared_ptr<Logger>& logger = CurrentLogger(); + if (logger) { + EmitIfEnabled(*logger, level, location, std::forward<MakeMessage>(make_message)); + } +} + +/// \brief Runtime-level variant against the current logger: emit if enabled, then +/// flush + abort when level == kFatal (using the same acquired logger). +template <typename MakeMessage> +void LogToCurrentRuntime(LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + const std::shared_ptr<Logger>& logger = CurrentLogger(); + if (logger) { + EmitIfEnabled(*logger, level, location, std::forward<MakeMessage>(make_message)); + } + if (level == LogLevel::kFatal) { + if (logger) logger->Flush(); + std::abort(); + } +} Review Comment: When ICEBERG_LOG(level, ...) or ICEBERG_LOG_RUNTIME_FMT(level, ...) is invoked with level == kFatal, the runtime path aborts without running the configured FatalHandler and only formats the message if ShouldLog(level) passes. This is inconsistent with ICEBERG_LOG_FATAL (which always formats for the handler and invokes it even when the record is suppressed). ########## src/iceberg/logging/log_macros.h: ########## @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/logging/log_macros.h +/// \brief Iceberg-prefixed logging macros (ICEBERG_LOG_*). +/// +/// Kept out of logger.h so consumers of the C++ logging API (Logger, Log(), +/// ScopedLogger) are not forced to pull in these macros or the conforming +/// preprocessor they require. Include this header to use the macros; include +/// short_log_macros.h for the bare LOG_* aliases. + +#include <cstdlib> +#include <format> +#include <memory> +#include <source_location> +#include <string> +#include <string_view> +#include <utility> + +#include "iceberg/logging/log_level.h" +#include "iceberg/logging/logger.h" + +namespace iceberg::internal { + +/// \brief Runtime (non-literal) format-string helper for ICEBERG_LOG_RUNTIME_FMT. +/// +/// std::format requires a compile-time format string; this routes a runtime +/// string through std::vformat. Args are bound as named lvalues and the +/// arg-store is held in a named variable so it outlives the vformat call +/// (C++23 make_format_args rejects rvalues -- P2905 / LWG3631). +template <typename... Args> +std::string VFormat(std::string_view fmt, Args&&... args) { + auto store = std::make_format_args(args...); + return std::vformat(fmt, store); +} + +/// \brief Gate on \p logger.ShouldLog, then format (via \p make_message) and emit. +/// +/// \p make_message is a callable returning the formatted std::string; it is +/// invoked only after the level passes ShouldLog, so a disabled log never +/// evaluates its format arguments. Never throws: a std::formatter that throws +/// (any type) routes to EmitFormatError, so the noexcept logging contract holds. +template <typename MakeMessage> +void EmitIfEnabled(Logger& logger, LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + if (!logger.ShouldLog(level)) return; + try { + Emit(logger, level, location, std::forward<MakeMessage>(make_message)()); + } catch (...) { + EmitFormatError(logger, level, location); + } +} + +/// \brief Emit to the current (scoped-or-default) logger if enabled. +template <typename MakeMessage> +void LogToCurrent(LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + const std::shared_ptr<Logger>& logger = CurrentLogger(); + if (logger) { + EmitIfEnabled(*logger, level, location, std::forward<MakeMessage>(make_message)); + } +} + +/// \brief Runtime-level variant against the current logger: emit if enabled, then +/// flush + abort when level == kFatal (using the same acquired logger). +template <typename MakeMessage> +void LogToCurrentRuntime(LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + const std::shared_ptr<Logger>& logger = CurrentLogger(); + if (logger) { + EmitIfEnabled(*logger, level, location, std::forward<MakeMessage>(make_message)); + } + if (level == LogLevel::kFatal) { + if (logger) logger->Flush(); + std::abort(); + } +} + +/// \brief Runtime-level variant against an explicit logger: emit if enabled, then +/// flush + abort when level == kFatal. +template <typename MakeMessage> +void LogToExplicitRuntime(Logger& logger, LogLevel level, + const std::source_location& location, + MakeMessage&& make_message) noexcept { + EmitIfEnabled(logger, level, location, std::forward<MakeMessage>(make_message)); + if (level == LogLevel::kFatal) { + logger.Flush(); + std::abort(); + } +} Review Comment: ICEBERG_LOG_TO(logger, kFatal, ...) currently flushes and aborts but does not run FatalHandler, and it does not guarantee the formatted message is available to a handler when the fatal record is filtered out. If FatalHandler is meant to be a process-wide termination hook, it should run for this fatal path as well. ########## src/iceberg/logging/internal/spdlog_logger.cc: ########## @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/logging/internal/spdlog_logger.h" + +#ifdef ICEBERG_HAS_SPDLOG + +# include <memory> +# include <string> +# include <unordered_map> +# include <utility> + +# include <spdlog/common.h> +# include <spdlog/sinks/stdout_color_sinks.h> + +namespace iceberg::internal { + +namespace { + +spdlog::level::level_enum ToSpdLevel(LogLevel level) noexcept { + switch (level) { + case LogLevel::kTrace: + return spdlog::level::trace; + case LogLevel::kDebug: + return spdlog::level::debug; + case LogLevel::kInfo: + return spdlog::level::info; + case LogLevel::kWarn: + return spdlog::level::warn; + case LogLevel::kError: + return spdlog::level::err; + case LogLevel::kCritical: + case LogLevel::kFatal: + // spdlog has no "fatal"; the process abort is owned by the macro layer. + return spdlog::level::critical; + case LogLevel::kOff: + return spdlog::level::off; + } + return spdlog::level::off; +} + +} // namespace + +SpdLogger::SpdLogger(LogLevel level) + : SpdLogger(std::make_shared<spdlog::logger>( + "iceberg", std::make_shared<spdlog::sinks::stderr_color_sink_mt>()), + level) {} + +Status SpdLogger::Initialize( + const std::unordered_map<std::string, std::string>& properties) { + if (auto it = properties.find(std::string(kPatternProperty)); it != properties.end()) { + logger_->set_pattern(it->second); + } + // Apply "level" via the base implementation. + return Logger::Initialize(properties); +} + +SpdLogger::SpdLogger(std::shared_ptr<spdlog::logger> logger, LogLevel level) + : logger_(std::move(logger)), level_(level) { + if (logger_) { + logger_->set_level(spdlog::level::trace); // filtering is done by ShouldLog + } +} Review Comment: SpdLogger(std::shared_ptr<spdlog::logger>, ...) accepts a potentially-null spdlog::logger, but Initialize/Log/Flush unconditionally dereference logger_. This can crash if a caller accidentally passes nullptr (tests or internal callers). Consider ensuring logger_ is non-null in the constructor (or guarding all dereferences). -- 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]
