Copilot commented on code in PR #726: URL: https://github.com/apache/iceberg-cpp/pull/726#discussion_r3612639141
########## 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, ...) aborts via LogToExplicitRuntime(), but this path does not run the registered FatalHandler and only formats the message if ShouldLog(kFatal) passes. That makes fatal-handler behavior inconsistent with ICEBERG_LOG_FATAL (and the FatalHandler documentation). ########## 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 + } +} + +void SpdLogger::Log(LogMessage&& message) noexcept { + try { + spdlog::source_loc loc{message.location.file_name(), + static_cast<int>(message.location.line()), + message.location.function_name()}; + // Pass the pre-formatted text as an argument ("{}") so any braces in the + // message are not re-interpreted as a format string. + logger_->log(loc, ToSpdLevel(message.level), "{}", message.message); + } catch (...) { + // Logging must never throw. + } +} Review Comment: SpdLogger::Log() dereferences logger_ without a null check. Since SpdLogger can be constructed with a null shared_ptr, this can crash instead of acting as a no-op logger. ########## src/iceberg/CMakeLists.txt: ########## @@ -17,6 +17,18 @@ set(ICEBERG_INCLUDES "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src>" "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>") + +# Generate the logging backend config header. ALWAYS generated (not gated by +# ICEBERG_SPDLOG) so logging/logger.cc can include it in both ON and OFF builds; +# only the definedness of ICEBERG_HAS_SPDLOG varies. Generated into the build +# tree (already on ICEBERG_INCLUDES), included as "iceberg/logging/config.h", and +# NOT installed (it must never appear in a public/installed header). +if(ICEBERG_SPDLOG) + set(ICEBERG_HAS_SPDLOG ON) +endif() +configure_file("${CMAKE_CURRENT_SOURCE_DIR}/logging/config.h.in" + "${CMAKE_CURRENT_BINARY_DIR}/logging/config.h") Review Comment: configure_file() writes to "${CMAKE_CURRENT_BINARY_DIR}/logging/config.h", but this CMakeLists.txt generates it before the add_subdirectory(logging) that would create the binary "logging" directory. If the directory doesn’t already exist, configure_file can fail at configure time. Create the directory explicitly before calling configure_file. ########## src/iceberg/logging/internal/spdlog_logger.h: ########## @@ -0,0 +1,89 @@ +/* + * 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/internal/spdlog_logger.h +/// \brief spdlog-backed logging sink. +/// +/// INTERNAL, NOT INSTALLED. It is only included from .cc files (logger.cc and +/// spdlog_logger.cc) after config.h, and only when the project is built with +/// ICEBERG_SPDLOG=ON. SpdLogger is not a consumer-constructible public type -- +/// applications obtain it via the default logger or the "logger-impl"="spdlog" +/// registry factory. + +#include "iceberg/logging/config.h" Review Comment: The header comment says this file is only included from .cc files "after config.h", but the header itself includes config.h unconditionally. This is internally inconsistent and may mislead future maintainers about required include order. ########## 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); +} Review Comment: SpdLogger::Initialize() unconditionally dereferences logger_ (set_pattern), but the constructor accepts a potentially-null std::shared_ptr<spdlog::logger> (and even checks for null when setting the spdlog level). If logger_ is null, Initialize() will crash. ########## 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 + } +} + +void SpdLogger::Log(LogMessage&& message) noexcept { + try { + spdlog::source_loc loc{message.location.file_name(), + static_cast<int>(message.location.line()), + message.location.function_name()}; + // Pass the pre-formatted text as an argument ("{}") so any braces in the + // message are not re-interpreted as a format string. + logger_->log(loc, ToSpdLevel(message.level), "{}", message.message); + } catch (...) { + // Logging must never throw. + } +} + +void SpdLogger::Flush() noexcept { + try { + logger_->flush(); + } catch (...) { + } +} Review Comment: SpdLogger::Flush() dereferences logger_ without a null check. If a null logger_ is ever constructed, Flush() will crash on the fatal path instead of being best-effort/no-op. ########## 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: ICEBERG_LOG(level, ...) aborts on kFatal via LogToCurrentRuntime(), but this path does not run the registered FatalHandler and also only formats the message if ShouldLog(kFatal) passes. This diverges from the FatalHandler contract in logger.h (handler receives the formatted message even when the fatal record is filtered). Align the runtime-level kFatal behavior with ICEBERG_LOG_FATAL by formatting unconditionally, flushing, running FatalHandler, then aborting. -- 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]
