Conversation
|
Warning Rate limit exceeded@kimkulling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a public cppcore::DateTime type and exports its header; integrates DateTime into Logger formatting, adds Logger::set(Logger*) to inject/replace the singleton, adjusts Logger teardown and source ordering; registers two new unit tests for DateTime and Logger in CMake. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Logger as Logger (singleton)
note over Logger: sLogger (singleton instance)
App->>Logger: set(customLogger*)
alt sLogger exists
Logger->>Logger: kill()\n(if sLogger != nullptr) delete sLogger
end
Logger->>Logger: sLogger = customLogger*
App-->>Logger: getInstance()/use
sequenceDiagram
autonumber
participant Logger
participant DateTime
participant Sink as Output Sink
Logger->>Logger: info("msg")
Logger->>Logger: getDateTime()
Logger->>DateTime: DateTime()
DateTime-->>Logger: {year, month, day, hour, min, sec}
Logger->>Logger: format "YYYY-MM-DD HH:MM:SS"
Logger->>Sink: write "[ts] msg"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
include/cppcore/Common/Logger.h (1)
108-111: Clarify ownership and thread-safety semantics for set.Specify whether Logger takes ownership of logger (deleted by kill()) and whether this API is thread-safe. Consider renaming to setInstance for clarity.
Suggested header comment tweak:
- /// @brief Will set a user-defined logger instance. - /// @param[in] logger The new logger instance. + /// @brief Set the singleton instance. + /// @param[in] logger New logger instance pointer. + /// @note Ownership: The logger pointer will be adopted and deleted by kill(). + /// @warning Not thread-safe. Call during single-threaded init only. static void set(Logger *logger);include/cppcore/Common/DateTime.h (3)
31-35: Incorrect doc tag and brief.The Doxygen header references THash and “class”; update to DateTime and clarify it captures local time at construction.
-/// @class THash +/// @class DateTime ... -/// @brief This class is used to get the current date and time. +/// @brief Represents the local date/time captured at construction.
37-43: Consider narrowing integer types and documenting ranges.year fits into uint16_t for current usage; document ranges (month 1–12, day 1–31, etc.) to prevent misuse.
25-27: Missing for uint32_t (if not guaranteed by CPPCoreCommon.h).Add explicitly to avoid transitive include dependency.
#include <cppcore/CPPCoreCommon.h> +#include <cstdint> #include <ctime>test/common/LoggerTest.cpp (1)
8-10: Exercise create/set/get/kill path and guard against same-instance set.Add a minimal test that would catch the set same-instance bug and validates lifecycle.
TEST_F(LoggerTest, CreateTest) { - + Logger *l = Logger::create(); + ASSERT_NE(nullptr, l); + // Setting the same instance should be a no-op (regression for potential UAF). + Logger::set(l); + EXPECT_EQ(Logger::getInstance(), l); + Logger::kill(); + EXPECT_EQ(Logger::getInstance(), nullptr); }code/Common/Logger.cpp (1)
106-109: Safe delete guard added—good.
Prevents accidental double delete whenkill()is called withnullptr.Consider making
create()/set()/kill()thread-safe with a mutex if Logger can be used across threads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CMakeLists.txt(2 hunks)code/Common/Logger.cpp(5 hunks)include/cppcore/Common/DateTime.h(1 hunks)include/cppcore/Common/Logger.h(1 hunks)test/common/DateTimeTest.cpp(1 hunks)test/common/LoggerTest.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/common/DateTimeTest.cpp (1)
include/cppcore/Common/DateTime.h (2)
cppcore(28-56)dt(47-47)
include/cppcore/Common/Logger.h (1)
code/Common/Logger.cpp (2)
set(92-95)set(92-92)
code/Common/Logger.cpp (1)
include/cppcore/Common/Logger.h (1)
getVerboseMode(126-189)
🔇 Additional comments (5)
CMakeLists.txt (2)
77-78: LGTM: DateTime header exported and Logger.cpp listed early.Public header is part of cppcore_common_src; good for consumers.
139-142: LGTM: Added DateTime and Logger tests.Ensure these files are included in the test target on all platforms (they are via cppcore_common_test_src).
code/Common/Logger.cpp (3)
24-24: Include for DateTime looks correct.
Enables the new timestamp formatting. No issues.
143-146: No functional change; readability improved.
Condition split across lines is fine.
255-268: I couldn’t locate any constructor implementation forDateTime—it appears to be a plain POD with no time-fetch logic in the header. Please verify:
- Whether
DateTime’s default ctor actually populates fields (UTC or local) or if there’s a missing implementation.- That changing padding to 4 for the year and 2 for all other components aligns with your desired ISO/UTC semantics.
Without confirming how
currentDateTimeis set, we risk formatting uninitialized/zero values.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
include/cppcore/Common/DateTime.h (1)
38-38: Month is zero-based; add +1 and update the comment.tm_mon is 0..11. Expose 1..12 to callers and document it.
- uint32_t month{}; ///< The current month + uint32_t month{}; ///< The current month (1..12) ... - month = dt.tm_mon; + month = static_cast<uint32_t>(dt.tm_mon + 1);Also applies to: 53-53
🧹 Nitpick comments (3)
include/cppcore/Common/DateTime.h (3)
46-52: Check localtime return values to handle failures.Harden against rare failures from localtime_s/localtime_r.
-#if defined(_WIN32) - ::localtime_s(&dt, ×tamp); -#else - ::localtime_r(×tamp, &dt); -#endif +#if defined(_WIN32) + const errno_t rc = ::localtime_s(&dt, ×tamp); + if (rc != 0) { + dt = tm{}; // zero-init on failure + } +#else + if (::localtime_r(×tamp, &dt) == nullptr) { + dt = tm{}; // zero-init on failure + } +#endif
25-27: Include cstdint explicitly for uint32_t.Avoid relying on transitive includes.
#include <cppcore/CPPCoreCommon.h> +#include <cstdint> #include <ctime>
44-45: Mark constructor noexcept.Construction from system time should not throw; improves usability.
- DateTime() { + DateTime() noexcept {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
code/Common/Logger.cpp(5 hunks)include/cppcore/Common/DateTime.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/Common/Logger.cpp
🔇 Additional comments (1)
include/cppcore/Common/DateTime.h (1)
1-23: License header and header guard look good.
Updated class documentation to reflect correct class name.
Added a placeholder comment for future implementation.
Added MIT License header to DateTimeTest.cpp
Added MIT License header to LoggerTest.cpp
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores