Repository navigation
refactor: improve logging - #301
Conversation
WalkthroughReplaced many direct logging:: calls with LOGGING_* macros and introduced err()/critical() and logging options; removed the public CLI Changes
Sequence Diagram(s)sequenceDiagram
participant Site as Call sites
participant Macro as LOGGING_* macro
participant Gate as LOGGING_MESSAGE
participant API as clice::logging::{info,err,critical}
participant Impl as Logging Impl (sinks)
Site->>Macro: LOGGING_INFO/DEBUG/WARN/ERROR(...)
Macro->>Gate: translate + level check
Gate->>Gate: if level permits
alt permitted
Gate->>API: info/err(...)
API->>Impl: emit record -> ringbuffer + sinks (stderr/file)
else
Gate-->>Site: no-op
end
rect rgb(255,245,235)
Note right of API: Critical path (noreturn)
Site->>Macro: LOGGING_FATAL(...)
Macro->>API: critical(...)
API->>Impl: emit CRITICAL, flush sinks
Impl->>Site: process exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
bin/clice.cc(3 hunks)include/Support/Logging.h(1 hunks)src/AST/Selection.cpp(0 hunks)src/Async/FileSystem.cpp(1 hunks)src/Async/Network.cpp(3 hunks)src/Async/libuv.cpp(1 hunks)src/Compiler/Command.cpp(6 hunks)src/Compiler/Tidy.cpp(2 hunks)src/Compiler/Toolchain.cpp(1 hunks)src/Feature/Diagnostic.cpp(1 hunks)src/Feature/Formatting.cpp(1 hunks)src/Server/Document.cpp(14 hunks)src/Server/Indexer.cpp(6 hunks)src/Server/Lifecycle.cpp(2 hunks)src/Server/Server.cpp(3 hunks)tests/unit/Index/USR.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/AST/Selection.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/Compiler/Command.cpp (1)
src/Server/Indexer.cpp (2)
index(10-59)index(10-10)
src/Server/Document.cpp (2)
include/Support/FileSystem.h (1)
path(14-23)src/Feature/Diagnostic.cpp (2)
diagnostics(8-120)diagnostics(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: build (macos-15)
🔇 Additional comments (16)
src/Async/FileSystem.cpp (1)
125-125: Logging macro replacement looks correct.The change from
logging::warntoLOGGING_WARNis consistent with the refactoring. However, this will be affected by the critical issue in theLOGGING_MESSAGEmacro implementation (see comments oninclude/Support/Logging.h).bin/clice.cc (1)
107-172: Logging macro replacements are consistent.All replacements from
logging::info/logging::warntoLOGGING_INFO/LOGGING_WARNare consistent with the refactoring approach. These changes will be affected by the critical bug in theLOGGING_MESSAGEmacro (see comments oninclude/Support/Logging.h).include/Support/Logging.h (2)
75-84: New logging functions look correct.The addition of
err()andcritical()functions to replace the removedfatal()function is well-structured. Thecritical()function correctly implements fatal logging with[[noreturn]]attribute and process termination.
93-98: LOGGING_FATAL correctly bypasses level checking.The
LOGGING_FATALmacro correctly callscritical()directly without going throughLOGGING_MESSAGE, ensuring fatal messages are always logged regardless of the configured level. This is the correct behavior for fatal errors.tests/unit/Index/USR.cpp (1)
39-39: Logging updates in tests look correct.The commented-out
LOGGING_INFOat line 39 is acceptable for test code. The replacement oflogging::fatalwithLOGGING_FATALat line 64 is correct and will work as expected sinceLOGGING_FATALdirectly callscritical()without level checking.Also applies to: 64-64
src/Async/libuv.cpp (1)
32-36: Logging macro replacements are consistent.The replacements from
logging::warntoLOGGING_WARNare consistent with the refactoring. These changes will be affected by the critical bug in theLOGGING_MESSAGEmacro (see comments oninclude/Support/Logging.h).src/Async/Network.cpp (1)
33-33: Fatal logging replacements are correct.The replacements from
logging::fataltoLOGGING_FATALare correct and will work as expected. SinceLOGGING_FATALdirectly callscritical()without level checking, these changes are not affected by the bug in theLOGGING_MESSAGEmacro.Also applies to: 60-60, 133-133
src/Feature/Diagnostic.cpp (1)
35-37: Logging macro replacement is consistent.The change from
logging::infotoLOGGING_INFOis consistent with the refactoring. This change will be affected by the critical bug in theLOGGING_MESSAGEmacro (see comments oninclude/Support/Logging.h).src/Feature/Formatting.cpp (1)
44-44: Logging macro replacement is consistent.The change from
logging::infotoLOGGING_INFOis consistent with the refactoring. This change will be affected by the critical bug in theLOGGING_MESSAGEmacro (see comments oninclude/Support/Logging.h).src/Server/Lifecycle.cpp (1)
6-8: LGTM! Clean logging macro migration.The logging calls have been correctly updated to use the new
LOGGING_*macro API. All format strings and parameters are preserved correctly, and the behavior remains unchanged.Also applies to: 20-20, 25-25, 27-28
src/Compiler/Tidy.cpp (1)
337-337: LGTM! Logging macros updated correctly.The migration from
logging::infotoLOGGING_INFOis clean and consistent with the project-wide refactoring.Also applies to: 341-341, 394-394
src/Compiler/Toolchain.cpp (1)
196-196: LGTM! Error logging updated correctly.The warning logs in the cleanup paths have been properly migrated to the new macro API.
Also applies to: 201-201
src/Server/Indexer.cpp (1)
18-18: LGTM! Comprehensive logging migration.All logging calls throughout the indexer have been correctly updated to use the new macro API. The multi-line format strings are handled properly.
Also applies to: 28-28, 58-58, 110-112, 120-122, 142-142, 150-150, 159-159, 164-164
src/Server/Document.cpp (1)
15-15: LGTM! Thorough logging refactoring.All logging calls have been systematically migrated to the new macro API. The use of
LOGGING_FATALat Line 307 is appropriate since it represents a critical logic error that should terminate the program (PCH expected but not built).Also applies to: 22-22, 33-33, 78-78, 108-108, 114-114, 121-121, 130-130, 135-135, 141-141, 181-181, 202-202, 223-225, 230-230, 257-257, 266-266, 271-271, 307-307, 327-329, 352-352, 366-366, 371-371
src/Server/Server.cpp (1)
127-127: LGTM! Server logging updated correctly.The logging migration is clean and appropriate. The use of
LOGGING_FATALfor invalid LSP messages correctly reflects that this is a critical error that should terminate the process.Also applies to: 138-138, 155-155, 163-163, 170-170, 173-173, 180-180
src/Compiler/Command.cpp (1)
179-182: LGTM! Compilation database logging refactored correctly.All logging calls have been properly migrated to the new macro API. The multi-line format strings are handled correctly, maintaining readability while using the new infrastructure.
Also applies to: 287-287, 495-496, 627-627, 633-633, 637-637, 645-646, 676-677, 714-714
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/clice.cc (1)
105-106: Update the log message to reflect the unconditional initialization.The message "No resource directory specified, using default resource directory" implies an optional configuration that no longer exists. Since the
--resource-diroption was removed in this PR, consider updating this to something like "Initializing default resource directory".Apply this diff to clarify the message:
- LOGGING_INFO("No resource directory specified, using default resource directory"); - // Try to initialize default resource directory + LOGGING_INFO("Initializing default resource directory"); if(auto result = fs::init_resource_dir(argv[0]); !result) {tests/conftest.py (1)
63-63: Remove the unusedtest_data_dirparameter.The
test_data_dirparameter is declared but never used within theclientfixture body. This creates unnecessary coupling in the pytest dependency graph.Apply this diff to remove the unused parameter:
-async def client(request, executable: Path | None, test_data_dir: Path): +async def client(request, executable: Path | None):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/clice.cc(3 hunks)bin/unit_tests.cc(2 hunks)tests/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bin/clice.cc (1)
src/Async/Network.cpp (4)
listen(71-85)listen(71-71)listen(87-108)listen(87-87)
🪛 Ruff (0.14.3)
tests/conftest.py
63-63: Unused function argument: test_data_dir
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, release)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (5)
bin/clice.cc (3)
100-100: LGTM! Logging macro migration.The replacement of
logging::infowithLOGGING_INFOis consistent with the PR's refactoring objective.
137-137: LGTM! Server startup logging updated.The logging macro replacements for server startup messages are consistent with the refactoring effort.
Also applies to: 143-143
155-155: LGTM! Exit logging updated.The logging macro replacement for the exit message is correct.
bin/unit_tests.cc (2)
21-21: LGTM! Capitalization consistency improvement.The change to lowercase "clice" maintains consistency with the naming convention used elsewhere in the codebase (e.g., bin/clice.cc).
195-198: LGTM! Resource directory initialization simplified.The unconditional initialization of the default resource directory with clear error handling aligns with the simplified approach taken in bin/clice.cc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/Support/Logging.h (2)
93-97: Consider using__VA_OPT__for better portability.These macros may produce trailing commas when called without variadic arguments (e.g.,
LOGGING_TRACE("msg")), expanding toLOGGING_MESSAGE(trace, "msg", ). While this might work with compiler extensions, it's not portable C++20.For consistency with
LOGGING_FATAL(line 98) and better portability:Apply this diff:
-#define LOGGING_TRACE(fmt, ...) LOGGING_MESSAGE(trace, fmt, __VA_ARGS__) -#define LOGGING_DEBUG(fmt, ...) LOGGING_MESSAGE(debug, fmt, __VA_ARGS__) -#define LOGGING_INFO(fmt, ...) LOGGING_MESSAGE(info, fmt, __VA_ARGS__) -#define LOGGING_WARN(fmt, ...) LOGGING_MESSAGE(warn, fmt, __VA_ARGS__) -#define LOGGING_ERROR(fmt, ...) LOGGING_MESSAGE(err, fmt, __VA_ARGS__) +#define LOGGING_TRACE(fmt, ...) LOGGING_MESSAGE(trace, fmt __VA_OPT__(, ) __VA_ARGS__) +#define LOGGING_DEBUG(fmt, ...) LOGGING_MESSAGE(debug, fmt __VA_OPT__(, ) __VA_ARGS__) +#define LOGGING_INFO(fmt, ...) LOGGING_MESSAGE(info, fmt __VA_OPT__(, ) __VA_ARGS__) +#define LOGGING_WARN(fmt, ...) LOGGING_MESSAGE(warn, fmt __VA_OPT__(, ) __VA_ARGS__) +#define LOGGING_ERROR(fmt, ...) LOGGING_MESSAGE(err, fmt __VA_OPT__(, ) __VA_ARGS__)
98-98: Minor inconsistency: trailing semicolon in macro definition.Unlike the other logging macros,
LOGGING_FATALincludes a trailing semicolon. This meansLOGGING_FATAL("msg");expands toclice::logging::critical("msg");;(double semicolon). While valid, consider removing the semicolon from the macro for consistency with the other macros.Apply this diff:
-#define LOGGING_FATAL(fmt, ...) clice::logging::critical(fmt __VA_OPT__(, ) __VA_ARGS__); +#define LOGGING_FATAL(fmt, ...) clice::logging::critical(fmt __VA_OPT__(, ) __VA_ARGS__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/Support/Logging.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: build (macos-15)
🔇 Additional comments (3)
include/Support/Logging.h (3)
75-77: LGTM! Newerrfunction is correctly implemented.The new error logging function follows the established pattern and correctly delegates to the underlying
logfunction with the appropriate level.
79-84: LGTM! Thecriticalfunction correctly implements fatal logging.The
[[noreturn]]attribute is properly applied, and the termination sequence (log → shutdown → exit) is appropriate for a fatal error handler.
88-91: Previous critical issue has been resolved.The level comparison logic now correctly uses
<=to implement hierarchical logging semantics. Messages at or above the configured threshold will now be logged as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/clice.cc(3 hunks)bin/unit_tests.cc(2 hunks)include/Support/Logging.h(2 hunks)src/Server/Lifecycle.cpp(2 hunks)src/Support/Logging.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/unit_tests.cc
🧰 Additional context used
🧬 Code graph analysis (3)
include/Support/Logging.h (1)
src/Support/Logging.cpp (4)
stderr_logger(17-32)stderr_logger(17-17)file_loggger(34-54)file_loggger(34-34)
bin/clice.cc (3)
src/Support/Logging.cpp (2)
stderr_logger(17-32)stderr_logger(17-17)src/Async/Network.cpp (4)
listen(71-85)listen(71-71)listen(87-108)listen(87-87)include/Support/Logging.h (1)
flush(23-25)
src/Server/Lifecycle.cpp (2)
src/Support/Logging.cpp (2)
file_loggger(34-54)file_loggger(34-34)include/Support/Logging.h (1)
flush(23-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: build (macos-15)
🔇 Additional comments (11)
include/Support/Logging.h (3)
23-25: LGTM!The inline
flush()helper provides a convenient way to flush the default logger.
86-95: LGTM!The new
err()andcritical()entry points are well-designed:
err()provides non-fatal error loggingcritical()correctly uses[[noreturn]], callsspdlog::shutdown(), and exits with code 1
99-109: LGTM!The logging macros are correctly implemented:
LOGGING_MESSAGEproperly gates by severity using<=(fixed from previous review)LOGGING_FATALappropriately bypasses the level check since it always terminates the program- The
__VA_OPT__usage correctly handles optional variadic argumentssrc/Server/Lifecycle.cpp (2)
6-8: LGTM!The migration to logging macros (
LOGGING_INFO,LOGGING_WARN,LOGGING_FATAL) is consistent and correct throughout the function.Also applies to: 20-20, 25-29
31-34: LGTM!The conditional file logging initialization is well-implemented:
- Only creates file logger when
logging_diris configured- Calls
flush()after switching to file logger to ensure console logs are persisted- This aligns with the ringbuffer replay feature
src/Support/Logging.cpp (3)
13-13: LGTM!The static ringbuffer sink and centralized pattern constant provide good infrastructure for the replay feature and ensure consistent formatting across loggers.
Also applies to: 15-15
17-32: LGTM!The updated
stderr_loggercorrectly implements the replay feature:
- Creates both console and ringbuffer sinks when
replay_consoleis enabled- Falls back to console-only when replay is disabled
- Applies the centralized pattern consistently
34-54: LGTM!The replay logic in
file_logggeris well-implemented:
- Replays buffered logs from the ringbuffer into the file sink when enabled
- Properly resets the ringbuffer after replay to free memory
- Applies consistent pattern and level settings
bin/clice.cc (3)
65-73: LGTM!The log level option updates are appropriate:
- Changed "fatal" to "error" to match the actual
Level::errenum- Added "off" option for disabling all logging
92-94: LGTM!The logging initialization is now streamlined and correctly uses the updated API.
101-103: LGTM!All logging calls have been successfully migrated to the new macro-based API (
LOGGING_INFO), andflush()is properly called at program exit to ensure all logs are written.Also applies to: 116-116, 122-122, 134-136
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/Support/Logging.h (2)
25-27: Function renames simplify the API.Removing the
create_prefix makes the API cleaner. Note: there's a typo infile_loggger(double 'g'), but this appears to be pre-existing.If desired, you could rename
file_logggertofile_loggerin a follow-up refactor to fix the typo.
100-105: Consider using__VA_OPT__in per-level macros for consistency.While the current implementation works (since
LOGGING_MESSAGEaccepts the trailing comma), adding__VA_OPT__(,)would make the macros more consistent with the innerLOGGING_MESSAGEmacro and clearer:#define LOGGING_TRACE(fmt, ...) LOGGING_MESSAGE(trace, fmt __VA_OPT__(,) __VA_ARGS__) #define LOGGING_DEBUG(fmt, ...) LOGGING_MESSAGE(debug, fmt __VA_OPT__(,) __VA_ARGS__) // ... and so onThis eliminates the trailing comma when
__VA_ARGS__is empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/clice.cc(3 hunks)include/Support/Format.h(1 hunks)include/Support/Logging.h(2 hunks)src/Server/Lifecycle.cpp(2 hunks)src/Support/Logging.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Support/Logging.cpp
🧰 Additional context used
🧬 Code graph analysis (4)
include/Support/Format.h (1)
include/Support/JSON.h (15)
Value(40-42)Value(44-46)Value(51-53)Value(63-65)Value(75-77)Value(87-89)Value(100-102)Value(112-114)Value(124-126)Value(131-133)Value(140-142)Value(154-156)Value(168-170)Value(182-184)Value(285-287)
src/Server/Lifecycle.cpp (2)
include/Support/JSON.h (1)
serialize(28-30)src/Support/Logging.cpp (2)
file_loggger(35-56)file_loggger(35-35)
bin/clice.cc (2)
src/Support/Logging.cpp (2)
stderr_logger(17-33)stderr_logger(17-17)src/Async/Network.cpp (4)
listen(71-85)listen(71-71)listen(87-108)listen(87-87)
include/Support/Logging.h (1)
src/Support/Logging.cpp (4)
stderr_logger(17-33)stderr_logger(17-17)file_loggger(35-56)file_loggger(35-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (14)
include/Support/Format.h (3)
76-76: LGTM: Public indent member is reasonable.The public
indentmember allows the indentation to be configured either through the format specification or programmatically. Since formatters are typically short-lived temporaries, this design is acceptable.
96-102: LGTM: Indentation correctly applied.The format method correctly uses
llvm::json::OStream(os, indent)to apply the configured indentation to the JSON output. This properly implements the indentation feature introduced by this change.
80-93: Integer overflow in indent parsing is possible but impractical; hardening is optional.Line 88 can overflow with extremely large format specs (e.g.,
{:999999999999}), though this is unlikely in practice since indent values are developer-controlled. If stricter safety is desired, add bounds checking before accumulation.The custom parse implementation is intentional, not a capability loss.
Unlike other formatters in this file (StringRef, Error, enum), the json::Value formatter doesn't delegate to Base::parse because standard format specs (width, alignment, fill) don't apply to JSON output. The specialized implementation—parsing only numeric indentation—is the correct design for this use case.
src/Server/Lifecycle.cpp (4)
6-8: LGTM! Logging macro migration is correct.The migration to
LOGGING_INFOis appropriate and the format string with client info is well-structured.
20-20: LGTM! Fatal logging is correctly applied.Using
LOGGING_FATALfor a missing workspace requirement is appropriate since the server cannot proceed without it.
25-29: LGTM! Improved config logging with better formatting.The migration to
LOGGING_*macros is correct, and the{0:4}format specifier adds 4-space indentation to the JSON output, improving readability of the config in logs.
31-33: LGTM! File logging initialization is well-placed.The conditional initialization ensures file logging is only set up when a directory is configured, and the placement after config loading allows the file logger to replay console logs (if
replay_consoleis enabled).include/Support/Logging.h (3)
11-21: LGTM! Options struct is well-designed.The configuration fields are clearly documented with sensible defaults. The
replay_consolefeature enables log history to be replayed to file sinks when they're created.
82-91: LGTM! Clear separation of error vs. critical logging.The distinction between
err()(non-fatal errors) andcritical()(fatal with exit) improves clarity. The[[noreturn]]attribute and proper shutdown sequence are correct.
95-98: LGTM! Level comparison is correctly fixed.The
<=operator properly implements hierarchical logging semantics (messages at or above the threshold are logged). The use of__VA_OPT__(, )correctly handles cases with zero variadic arguments.bin/clice.cc (4)
65-72: LGTM! Log level options align with the new logging API.Replacing "fatal" with "error" and adding "off" correctly reflects the new logging levels where
erris non-fatal andcriticalis the fatal level. The "off" option is a useful addition for disabling logging.
92-94: LGTM! Logging initialization is simplified and clear.Direct initialization of
logging::optionsand logger creation is more straightforward than the previous approach with separate helper functions.
96-98: LGTM! Fatal error handling is now correct.Using
LOGGING_FATALfor resource directory initialization failure is appropriate and consistent with the severity of the error (the program cannot proceed). This correctly addresses the previous review comment.
100-102: LGTM! All informational logging correctly uses LOGGING_INFO.The migration to
LOGGING_INFOis consistent throughout, and the log messages provide useful diagnostic information for server startup and shutdown.Also applies to: 115-115, 121-121, 133-133
Summary by CodeRabbit