Repository navigation
fix: Enhance logging examples and configuration for cross-platform use - #20
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors VNeLogging to support level-aware sink flushing, adds a configurable file-append option and an application override for platform log directory selection, refines ANSI color detection, updates tests/mocks, and adds a desktop-only platform logging example with CMake wiring. ChangesLogging Infrastructure Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/vertexnova/logging/logging.h (1)
135-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new
appendparameter in the public API comment.Line 143 adds
append, but the Doxygen block above it has no@param append, so the header docs are incomplete for callers.Suggested doc fix
* `@param` logger_name The name of the logger to which the file sink will be added. * `@param` file The name of the file where logs will be written. + * `@param` append Whether to append to an existing file (true) or truncate it (false). */ static void addFileSink(const std::string& logger_name, const std::string& file, bool append = true);As per coding guidelines, "Ensure public C++ APIs have brief doc comments if behavior isn't obvious."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/vertexnova/logging/logging.h` around lines 135 - 143, The Doxygen for addFileSink is missing documentation for the new parameter append; update the comment block for the static method addFileSink(const std::string& logger_name, const std::string& file, bool append = true) to include an `@param` append line describing that this flag (default true) controls whether the file sink opens the log file in append mode (true) or truncates/overwrites it (false), and mention the default behavior.
🧹 Nitpick comments (3)
include/vertexnova/logging/logging.h (2)
193-217: ⚡ Quick winAdd thread-safety contract for
setAppLogDirectory.The new public API on Line 217 mutates shared global logging state, but the docs do not state whether it is thread-safe or when it is safe to call.
Suggested doc fix
* `@param` path Absolute writable path for log files. Empty string resets to auto-detection. + * + * `@note` Thread-safety: call during startup before logger configuration. + * Concurrent calls with logging/configuration APIs are not guaranteed to be safe. */ static void setAppLogDirectory(const std::string& path);As per coding guidelines, "Flag missing or incorrect thread-safety documentation for public C++ APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/vertexnova/logging/logging.h` around lines 193 - 217, Document the thread-safety contract for setAppLogDirectory: state explicitly whether setAppLogDirectory(const std::string& path) is thread-safe or must only be called from a single thread during process initialization, and clarify its relationship with configureLogger() (e.g., "call before configureLogger() and not concurrently with any logging calls"). If it is not thread-safe, instruct callers to synchronize externally (or mention that the implementation acquires an internal mutex if it is thread-safe). Also mention the empty-string behavior (resets to auto-detection) and that callers on mobile platforms should call this from their platform startup code before starting other threads that may log.
280-280: ⚡ Quick winAlign private member naming with project C++ convention.
Line 280 introduces
s_app_log_dir, which does not follow the private-member convention requiringsnake_casewith a trailing underscore.As per coding guidelines, "Use snake_case + trailing underscore for C++ private members."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/vertexnova/logging/logging.h` at line 280, The static private member s_app_log_dir violates the project's private-member naming rule (snake_case + trailing underscore); rename it to s_app_log_dir_ (or app_log_dir_ if you prefer dropping the s_ prefix) and update all declarations, definitions, and usages (e.g., references in setAppLogDirectory, any getters, and .cpp definitions) to the new identifier so the symbol remains static and behavior is unchanged while matching the naming convention.examples/06_platform_logging/main.cpp (1)
97-97: ⚡ Quick winUse
std::filesystem::pathjoin instead of hardcoded"/"in cross-platform example.Hardcoding separators in a cross-platform sample adds avoidable platform coupling.
🔧 Suggested refactor
+#include <filesystem> ... - config.file_path = log_dir + "/platform_example.log"; + config.file_path = (std::filesystem::path(log_dir) / "platform_example.log").string();As per coding guidelines, "Avoid introducing C++ platform-specific behavior without guards and tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/06_platform_logging/main.cpp` at line 97, The code sets config.file_path by concatenating log_dir + "/platform_example.log", which hardcodes a path separator; update this to use std::filesystem::path to join paths portablely by constructing a std::filesystem::path from log_dir and appending "platform_example.log" (e.g., std::filesystem::path(log_dir) / "platform_example.log") and then assign the resulting path.string() (or path.native()) to config.file_path so the change touches the assignment involving config.file_path and the variable log_dir.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vertexnova/logging/core/log_dispatcher.h`:
- Line 86: Change the API so callers must explicitly choose what to flush:
remove the default parameter on LogDispatcher::flush (the declaration of void
flush(const std::vector<std::unique_ptr<ILogSink>>& log_sinks, LogLevel level))
so it no longer defaults to LogLevel::eTrace, and add an explicit helper
overload or method (e.g., flushAll or flush(const
std::vector<std::unique_ptr<ILogSink>>&) ) if you need the old “full flush”
behavior; update callers of LogDispatcher::flush and any call sites that relied
on the implicit default to pass the intended LogLevel or call the new flushAll
helper.
In `@src/vertexnova/logging/core/text_color.cpp`:
- Around line 18-20: The translation unit uses stdout/fileno inside
detectColorSupport() but doesn't include <cstdio>/<stdio.h>, risking missing
declarations on some platforms; add an explicit `#include` <cstdio> (or <stdio.h>)
at the top of src/vertexnova/logging/core/text_color.cpp (within the same
non-Windows include block or unconditionally) so detectColorSupport() can safely
call fileno(stdout) and isatty(fileno(stdout)).
In `@src/vertexnova/logging/logging.cpp`:
- Around line 223-228: The static s_app_log_dir must be protected with a mutex
to avoid data races: add a static std::mutex (e.g., s_app_log_dir_mutex) to the
Logging class and include <mutex>, then take a lock_guard<std::mutex> when
writing in Logging::setAppLogDirectory and when reading in
Logging::getPlatformSpecificLogDirectory (and any other accessors) so
ensureLogDirectoryExists is still called while holding the lock or called on a
local copy after locking; update references to use the locked access to
s_app_log_dir.
---
Outside diff comments:
In `@include/vertexnova/logging/logging.h`:
- Around line 135-143: The Doxygen for addFileSink is missing documentation for
the new parameter append; update the comment block for the static method
addFileSink(const std::string& logger_name, const std::string& file, bool append
= true) to include an `@param` append line describing that this flag (default
true) controls whether the file sink opens the log file in append mode (true) or
truncates/overwrites it (false), and mention the default behavior.
---
Nitpick comments:
In `@examples/06_platform_logging/main.cpp`:
- Line 97: The code sets config.file_path by concatenating log_dir +
"/platform_example.log", which hardcodes a path separator; update this to use
std::filesystem::path to join paths portablely by constructing a
std::filesystem::path from log_dir and appending "platform_example.log" (e.g.,
std::filesystem::path(log_dir) / "platform_example.log") and then assign the
resulting path.string() (or path.native()) to config.file_path so the change
touches the assignment involving config.file_path and the variable log_dir.
In `@include/vertexnova/logging/logging.h`:
- Around line 193-217: Document the thread-safety contract for
setAppLogDirectory: state explicitly whether setAppLogDirectory(const
std::string& path) is thread-safe or must only be called from a single thread
during process initialization, and clarify its relationship with
configureLogger() (e.g., "call before configureLogger() and not concurrently
with any logging calls"). If it is not thread-safe, instruct callers to
synchronize externally (or mention that the implementation acquires an internal
mutex if it is thread-safe). Also mention the empty-string behavior (resets to
auto-detection) and that callers on mobile platforms should call this from their
platform startup code before starting other threads that may log.
- Line 280: The static private member s_app_log_dir violates the project's
private-member naming rule (snake_case + trailing underscore); rename it to
s_app_log_dir_ (or app_log_dir_ if you prefer dropping the s_ prefix) and update
all declarations, definitions, and usages (e.g., references in
setAppLogDirectory, any getters, and .cpp definitions) to the new identifier so
the symbol remains static and behavior is unchanged while matching the naming
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e83e07e-cb7c-4906-8c40-06c4257f6c36
📒 Files selected for processing (22)
examples/06_platform_logging/CMakeLists.txtexamples/06_platform_logging/main.cppexamples/CMakeLists.txtinclude/vertexnova/logging/logging.hsrc/vertexnova/logging/core/async_logger.cppsrc/vertexnova/logging/core/console_log_sink.cppsrc/vertexnova/logging/core/console_log_sink.hsrc/vertexnova/logging/core/file_log_sink.cppsrc/vertexnova/logging/core/file_log_sink.hsrc/vertexnova/logging/core/log_dispatcher.cppsrc/vertexnova/logging/core/log_dispatcher.hsrc/vertexnova/logging/core/log_sink.hsrc/vertexnova/logging/core/sync_logger.cppsrc/vertexnova/logging/core/text_color.cppsrc/vertexnova/logging/log_manager.cppsrc/vertexnova/logging/log_manager.hsrc/vertexnova/logging/logging.cpptests/core/console_log_sink_test.cpptests/core/file_log_sink_test.cpptests/core/log_dispatcher_test.cpptests/core/mocks/log_sink_mock.htests/logging_path_test.cpp
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the logging library’s flushing semantics and file-sink configuration, and adds a new cross-platform example for initializing logging with an app-provided log directory (for sandboxed/mobile targets).
Changes:
- Extend sink flushing to accept a
LogLevelso console sinks can flush the relevant stream (stdoutvsstderr). - Add
LoggerConfig::file_appendand plumb it throughLogging::configureLogger()/LogManager::addFileSink()to control append vs truncate. - Add
Logging::setAppLogDirectory()+ a new example demonstrating platform-specific initialization.
BLOCKER (must fix before merge)
- src/vertexnova/logging/core/text_color.cpp (includes / isatty+fileno usage):
isatty(fileno(stdout))is used without including<cstdio>/<stdio.h>, which can cause compilation failures becausestdout/filenomay be undeclared. Add the proper header and (optionally) qualify with::stdout/::fileno.
MAJOR (strongly recommended)
- include/vertexnova/logging/logging.h (LoggerConfig default): Defaulting
flush_leveltoeTracemakes every log flush (since all levels are>= eTrace), which can significantly reduce logging throughput for both sync and async modes. Consider restoring the previous default (e.g.,eError) and letting users opt in to aggressive flushing. - src/vertexnova/logging/core/console_log_sink.cpp (flush semantics):
ConsoleLogSink::flush(level)currently flushes only one stream. Ifflush_levelis configured aseError, an error-triggered flush won’t flush priorstdoutlogs (buffered) and they can be lost on crash/abort. Consider flushing bothstd::coutandstd::cerrwhenlevel >= eError(or redefining console flush to “flush all”). - src/vertexnova/logging/core/log_sink.h (public API break): Changing
ILogSink::flush()toflush(LogLevel)is source-breaking for downstream custom sinks and any call sites usingflush(). If compatibility is a concern, consider keepingflush()and addingflush(LogLevel)with a default implementation that forwards toflush(), plus migration/deprecation guidance. - src/vertexnova/logging/logging.cpp (new behaviors lack tests):
file_appendbehavior (truncate vs append) should be covered by a deterministic unit test.setAppLogDirectory()override/reset behavior should be validated (override takes effect; empty string resets to auto-detection).
MINOR (nice-to-have)
- examples/06_platform_logging/main.cpp (Swift snippet): The Swift bridging snippet is misleading about
@_silgen_nameand direct calls into C++ from Swift. Adjust the snippet to show a C/extern "C"wrapper implemented in C++ that Swift calls, and keep the Swift sample focused on resolving the directory and passing it to that wrapper.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/logging_path_test.cpp | Updates default config expectation for new flush default. |
| tests/core/mocks/log_sink_mock.h | Updates mock sink flush signature to accept LogLevel. |
| tests/core/log_dispatcher_test.cpp | Updates dispatcher flush test to pass LogLevel. |
| tests/core/file_log_sink_test.cpp | Updates file sink flush calls to pass a LogLevel. |
| tests/core/console_log_sink_test.cpp | Updates console sink flush call to pass a LogLevel. |
| src/vertexnova/logging/logging.cpp | Adds setAppLogDirectory, plumbs file_append, updates default flush level and platform log dir behavior. |
| src/vertexnova/logging/log_manager.h | Extends addFileSink with an append parameter (defaulted). |
| src/vertexnova/logging/log_manager.cpp | Passes append into FileLogSink construction. |
| src/vertexnova/logging/core/text_color.cpp | Adds runtime color detection via isatty, plus platform exclusions. |
| src/vertexnova/logging/core/sync_logger.cpp | Forwards log level into sink flush calls; manual flush now flushes both streams via two calls. |
| src/vertexnova/logging/core/log_sink.h | Changes sink flush interface to flush(LogLevel). |
| src/vertexnova/logging/core/log_dispatcher.h | Extends dispatcher flush API to accept LogLevel (defaulted). |
| src/vertexnova/logging/core/log_dispatcher.cpp | Forwards LogLevel into sink flush calls. |
| src/vertexnova/logging/core/file_log_sink.h | Updates file sink flush signature to accept LogLevel (unused). |
| src/vertexnova/logging/core/file_log_sink.cpp | Implements flush(LogLevel) for file sink. |
| src/vertexnova/logging/core/console_log_sink.h | Updates console sink flush signature to accept LogLevel. |
| src/vertexnova/logging/core/console_log_sink.cpp | Flushes either cout or cerr based on log level. |
| src/vertexnova/logging/core/async_logger.cpp | Passes triggering log level into dispatcher flush calls; manual flush flushes both streams via two calls. |
| include/vertexnova/logging/logging.h | Adds file_append, changes default flush level, adds setAppLogDirectory API + docs. |
| examples/CMakeLists.txt | Adds new example subdirectory. |
| examples/06_platform_logging/main.cpp | New example demonstrating platform-specific log dir handling. |
| examples/06_platform_logging/CMakeLists.txt | Adds desktop-only build for the new example. |
Comments suppressed due to low confidence (1)
src/vertexnova/logging/logging.cpp:212
- New behavior: configureLogger now forwards LoggerConfig::file_append into addFileSink (truncation vs append), but there are no tests covering that the log file is actually truncated when file_append=false (or appended when true). Add a deterministic test (e.g., write once, reconfigure with same path + file_append=false, write again, then assert the old content was removed).
// Configure file sink (not on web)
#ifndef VNE_PLATFORM_WEB
if (cfg.sink == LogSinkType::eFile || cfg.sink == LogSinkType::eBoth) {
if (!cfg.file_path.empty()) {
addFileSink(cfg.name, cfg.file_path, cfg.file_append);
if (!cfg.file_pattern.empty()) {
setFilePattern(cfg.name, cfg.file_pattern);
}
}
}
…or cross-platform compatibility
…arameter for improved clarity
…t function for improved clarity
…for Swift integration
…ream behavior for different log levels
…or improved logging behavior
…onsistency and simplify usage
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
Tests