Add spdlog #5020
Add spdlog #5020
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a structured logging system using spdlog to replace direct console output (std::cout) throughout the codebase. The new logger provides log levels (TRACE, DEBUG, INFO, WARNING, ERROR, CRITICAL), file rotation, formatted output, and signal-safe shutdown handling.
Key changes:
- Added spdlog dependency and new Logger class with configurable log levels
- Replaced
std::coutandfmt::printstatements withg_logger()calls - Added Lua bindings for logging functions (
logInfo,logWarning,logError)
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json, CMakeLists.txt | Added spdlog dependency |
| src/logger.h, src/logger.cpp | New logger implementation using spdlog |
| src/otserv.cpp | Logger initialization and converted startup messages to use logger |
| src/script.cpp, src/scriptmanager.cpp | Converted script loading messages to use logger |
| src/luascript.h, src/luascript.cpp | Added Lua bindings for logging functions |
| src/configmanager.h, src/configmanager.cpp | Added LOG_LEVEL configuration option |
| src/databasemanager.cpp | Converted database messages to use logger |
| src/iomap.cpp, src/iomapserialize.cpp | Converted map loading messages to use logger |
| data/scripts/xml/actions.lua, data/scripts/globalevents/raids/raids.lua, data/lib/debugging/lua_version.lua | Replaced io.write/print with new Lua logging functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
src/logger.h
Outdated
| if (isEnabled(LogLevel::TRACE)) { | ||
| log(LogLevel::TRACE, msg); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Nah, if there's a configuration entry for this, it should not be controlled by compile flags
src/logger.h
Outdated
| virtual void log(LogLevel level, std::string_view message) = 0; | ||
| }; | ||
|
|
||
| Logger& g_logger(); |
There was a problem hiding this comment.
If it's a function, call it getLogger. g_ is reserved for global variables (🤮)
| default: | ||
| return LogLevel::INFO; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not directly use the spdlog enum?
src/logger.cpp
Outdated
| std::filesystem::path path(basePath); | ||
| std::string directory = path.parent_path().string(); | ||
| std::string baseName = path.stem().string(); | ||
| std::string extension = path.extension().string(); |
There was a problem hiding this comment.
None of these need to be converted to string
src/logger.cpp
Outdated
| } | ||
| catch (const std::exception& e) { | ||
| fprintf(stderr, "Error creating logger: %s\n", e.what()); | ||
| throw; |
There was a problem hiding this comment.
Rethrow if you won't handle, this will swallow the stack
src/logger.cpp
Outdated
|
|
||
| class LogWithSpdLog final : public Logger { | ||
| public: | ||
| LogWithSpdLog(std::string_view filePath, size_t rotateSize, size_t rotateFiles) { |
There was a problem hiding this comment.
This belongs to a free function createLogger that does all the preparation and then creates a new LogWithSpdlog when everything is checked
There was a problem hiding this comment.
Could we use std::format facilities over fmt::format?
|
Logging is a great addition, but do we need to wrap it? We could use spdlog directly, the API seems quite clean and easy |
3 participants
terminal
file .log
test in windows , vs solution
I have a code in python that modifies all
std::cout << xxxx << std::endl;tog_logger().info(xxxx)but because it would make the review more difficult, I will do it later.