Feature/add logger #40
Conversation
WalkthroughA logging system was introduced to the project, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Logger (Singleton)
participant LogStream(s)
User->>Logger (Singleton): log_<level>(domain, message)
Logger (Singleton)->>Logger (Singleton): Format message (add domain, severity, trace info)
Logger (Singleton)->>LogStream(s): Write formatted message
Estimated code review effort3 (~45 minutes) Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 10
🧹 Nitpick comments (3)
code/Common/Logger.cpp (2)
256-270: Implement getDateTime functionalityThe
getDateTimemethod is stubbed out and always returns "none", making the timestamp feature non-functional.Would you like me to implement a proper datetime formatting function using standard C++ chrono library? This would provide actual timestamps for log messages when
PrintMode::WithDateTimeis used.
48-55: Optimize stripFilename for cross-platform compatibilityThe current implementation only handles forward slashes and could be more efficient.
Consider using
std::filesystemfor better cross-platform support:-static ::std::string stripFilename(const ::std::string &filename) { - if (filename.empty()) { - return filename; - } - - ::std::string::size_type pos = filename.find_last_of("/"); - if (pos == ::std::string::npos) { - return filename; - } - const ::std::string strippedName = filename.substr(pos + 1, filename.size() - pos - 1); - - return strippedName; -} +static ::std::string stripFilename(const ::std::string &filename) { + if (filename.empty()) { + return filename; + } + // Handle both forward and backward slashes + ::std::string::size_type pos = filename.find_last_of("/\\"); + if (pos == ::std::string::npos) { + return filename; + } + return filename.substr(pos + 1); +}include/cppcore/Common/Logger.h (1)
207-252: Fix documentation inconsistenciesThe documentation for the logging macros references
osre_trace,osre_debug, etc., but the actual macro names arelog_trace,log_debug, etc.Update the documentation to match the actual macro names:
-/// @fn osre_trace +/// @fn log_traceAnd fix the typo "writhe" → "write" in lines 234, 242, and 250.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)code/Common/Logger.cpp(1 hunks)include/cppcore/Common/Logger.h(1 hunks)include/cppcore/Common/TStringBase.h(3 hunks)
🔇 Additional comments (2)
CMakeLists.txt (1)
78-79: Build configuration looks good!The Logger header and source files are correctly added to the
cppcore_common_srcgroup, maintaining consistency with the project structure.include/cppcore/Common/Logger.h (1)
143-143: Update default parameter after fixing enum typoAfter fixing the
WhithoutDateTimetypo, update this line as well.
| inline void TStringBase<T>::copyFrom(TStringBase<T> &base, const T *ptr, size_t size) { | ||
| if (ptr != nullptr) { | ||
| T *targetPtr = base.mBuffer; | ||
| if (size > 0) { | ||
| if (size > base.mCapacity) { | ||
| if (base.mStringBuffer != nullptr) { | ||
| delete [] base.mStringBuffer; | ||
| } | ||
| base.mStringBuffer = new T[size]; | ||
| base.mCapacity = size; | ||
| targetPtr = base.mStringBuffer; | ||
| } | ||
| memcpy(targetPtr, ptr, size * sizeof(T)); | ||
| base.mSize = size; | ||
| if (ptr == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| T *targetPtr = base.mBuffer; | ||
| if (size == 0) { | ||
| return; | ||
| } | ||
|
|
||
| if (size > base.mCapacity) { | ||
| if (base.mStringBuffer != nullptr) { | ||
| delete [] base.mStringBuffer; | ||
| } | ||
| base.mStringBuffer = new T[size]; | ||
| base.mCapacity = size; | ||
| targetPtr = base.mStringBuffer; | ||
| } | ||
| memcpy(targetPtr, ptr, size * sizeof(T)); | ||
| base.mSize = size; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
copyFrom doesn't ensure null termination
The copyFrom method uses memcpy but doesn't ensure null termination for C-style strings, which could lead to issues when using c_str().
Add null termination after copying:
memcpy(targetPtr, ptr, size * sizeof(T));
base.mSize = size;
+ // Ensure null termination
+ if (base.mStringBuffer != nullptr) {
+ base.mStringBuffer[size] = '\0';
+ } else if (size < InitSize) {
+ base.mBuffer[size] = '\0';
+ }Also, when allocating mStringBuffer, allocate size + 1 to accommodate the null terminator:
- base.mStringBuffer = new T[size];
+ base.mStringBuffer = new T[size + 1];
- base.mCapacity = size;
+ base.mCapacity = size + 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline void TStringBase<T>::copyFrom(TStringBase<T> &base, const T *ptr, size_t size) { | |
| if (ptr != nullptr) { | |
| T *targetPtr = base.mBuffer; | |
| if (size > 0) { | |
| if (size > base.mCapacity) { | |
| if (base.mStringBuffer != nullptr) { | |
| delete [] base.mStringBuffer; | |
| } | |
| base.mStringBuffer = new T[size]; | |
| base.mCapacity = size; | |
| targetPtr = base.mStringBuffer; | |
| } | |
| memcpy(targetPtr, ptr, size * sizeof(T)); | |
| base.mSize = size; | |
| if (ptr == nullptr) { | |
| return; | |
| } | |
| T *targetPtr = base.mBuffer; | |
| if (size == 0) { | |
| return; | |
| } | |
| if (size > base.mCapacity) { | |
| if (base.mStringBuffer != nullptr) { | |
| delete [] base.mStringBuffer; | |
| } | |
| base.mStringBuffer = new T[size]; | |
| base.mCapacity = size; | |
| targetPtr = base.mStringBuffer; | |
| } | |
| memcpy(targetPtr, ptr, size * sizeof(T)); | |
| base.mSize = size; | |
| } | |
| inline void TStringBase<T>::copyFrom(TStringBase<T> &base, const T *ptr, size_t size) { | |
| if (ptr == nullptr) { | |
| return; | |
| } | |
| T *targetPtr = base.mBuffer; | |
| if (size == 0) { | |
| return; | |
| } | |
| if (size > base.mCapacity) { | |
| if (base.mStringBuffer != nullptr) { | |
| delete [] base.mStringBuffer; | |
| } | |
| // Allocate an extra slot for the null terminator | |
| base.mStringBuffer = new T[size + 1]; | |
| base.mCapacity = size + 1; | |
| targetPtr = base.mStringBuffer; | |
| } | |
| memcpy(targetPtr, ptr, size * sizeof(T)); | |
| base.mSize = size; | |
| // Ensure null termination | |
| if (base.mStringBuffer != nullptr) { | |
| base.mStringBuffer[size] = '\0'; | |
| } else if (size < InitSize) { | |
| base.mBuffer[size] = '\0'; | |
| } | |
| } |
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h lines 162 to 182, the copyFrom method
copies data using memcpy but does not ensure null termination for C-style
strings, which can cause issues with c_str(). Fix this by allocating
mStringBuffer with size + 1 when resizing, and after copying, explicitly set the
character at index size to the null terminator to guarantee proper string
termination.
| template <class T> | ||
| inline TStringBase<T> &TStringBase<T>::operator += (const TStringBase<T> &rhs) { | ||
| if (rhs.isEmpty()) { | ||
| return *this; | ||
| } | ||
|
|
||
| copyFrom(*this, rhs.c_str(), rhs.size()); | ||
|
|
||
| return *this; | ||
| } |
There was a problem hiding this comment.
operator+= replaces content instead of appending
The operator+= implementation uses copyFrom which replaces the entire string content rather than appending to it. This is incorrect behavior for a concatenation operator.
The operator should append the content, not replace it. Consider implementing proper string concatenation logic that:
- Calculates the new total size
- Reallocates if necessary
- Copies the existing content and appends the new content
- Updates the size accordingly
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h around lines 203 to 212, the operator+=
incorrectly replaces the string content instead of appending. To fix this,
modify the operator to calculate the new total size by adding the current size
and rhs size, reallocate the internal buffer if needed to accommodate the new
size, copy the existing content followed by the rhs content into the buffer, and
update the size member to reflect the concatenated string length before
returning *this.
| template <class T> | ||
| inline T TStringBase<T>::operator[](size_t index) const { | ||
| return mBuffer[index]; | ||
| } |
There was a problem hiding this comment.
Add bounds checking to operator[]
The index operator doesn't validate the index against the string size, which could lead to out-of-bounds access.
Add bounds checking:
template <class T>
inline T TStringBase<T>::operator[](size_t index) const {
+ if (index >= mSize) {
+ // Handle error - throw exception or return default value
+ throw std::out_of_range("Index out of bounds");
+ }
- return mBuffer[index];
+ return c_str()[index];
}Also note that the current implementation always returns from mBuffer, but it should use c_str() to handle cases where mStringBuffer is in use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <class T> | |
| inline T TStringBase<T>::operator[](size_t index) const { | |
| return mBuffer[index]; | |
| } | |
| template <class T> | |
| inline T TStringBase<T>::operator[](size_t index) const { | |
| if (index >= mSize) { | |
| // Handle error - throw exception or return default value | |
| throw std::out_of_range("Index out of bounds"); | |
| } | |
| return c_str()[index]; | |
| } |
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h around lines 220 to 223, the operator[]
lacks bounds checking and always accesses mBuffer directly, which risks
out-of-bounds errors and ignores cases where mStringBuffer is used. Modify
operator[] to first check if the index is within the valid range of the string
size, throwing an exception or handling the error if not. Then, access the
character via c_str() instead of mBuffer to correctly handle both buffer storage
scenarios.
Fix review findings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
code/Common/Logger.cpp (2)
156-160: Refactor fragile indentation logic.The hardcoded character positions (6, 7) for detecting indentation markers remain fragile and should be refactored as previously suggested.
184-188: Refactor fragile indentation logic.The hardcoded character positions (6, 7) for detecting indentation markers remain fragile and should be refactored as previously suggested.
🧹 Nitpick comments (4)
code/Common/Logger.cpp (4)
83-106: Consider thread safety for the singleton pattern.The singleton implementation is correct for single-threaded use. For multi-threaded environments, consider using
std::once_flagor similar mechanisms to ensure thread-safe initialization.
138-148: Simplify the verbosity condition.The condition for
info()can be simplified since it covers most verbosity modes except potentially a hypothetical "Silent" mode.Consider simplifying to:
-if (getVerboseMode() == VerboseMode::Normal || getVerboseMode() == VerboseMode::Verbose || getVerboseMode() == VerboseMode::Debug || getVerboseMode() == VerboseMode::Trace) { +if (getVerboseMode() != VerboseMode::Silent) { // assuming Silent mode existsOr use a numeric comparison if verbosity levels are ordered.
231-236: Consider ownership semantics for log streams.The
unregisterLogStreammethod deletes the stream, which assumes the Logger owns all registered streams. This could cause issues if streams are shared or managed externally. Consider documenting this ownership model or providing separate methods for owned vs. unowned streams.
272-312: Consider refactoring to reduce code duplication.All helper print functions follow the same pattern. Consider extracting the common logic into a template or helper function to reduce duplication.
Example approach:
template<typename LogMethod> void printHelper(LogMethod method, const String &domain, const String &file, int line, const String &msg) { String message = msg; addTraceInfo(file, line, message); (Logger::getInstance()->*method)(domain, message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/Common/Logger.cpp(1 hunks)include/cppcore/Common/Logger.h(1 hunks)include/cppcore/Common/TStringBase.h(3 hunks)
🪛 GitHub Actions: CMake
code/Common/Logger.cpp
[warning] 253-253: Compiler warning: unused variable ‘Space’ [-Wunused-variable]
🚧 Files skipped from review as they are similar to previous changes (2)
- include/cppcore/Common/TStringBase.h
- include/cppcore/Common/Logger.h
🧰 Additional context used
🪛 GitHub Actions: CMake
code/Common/Logger.cpp
[warning] 253-253: Compiler warning: unused variable ‘Space’ [-Wunused-variable]
🔇 Additional comments (7)
code/Common/Logger.cpp (7)
1-30: LGTM!The license header and includes are properly structured and appropriate for the logger implementation.
32-69: Well-implemented helper functions.The static helper functions are correctly implemented with proper error handling:
appendDomainsafely handles empty domainsstripFilenameproperly handles edge casesaddTraceInfoconditionally adds trace information based on verbosity mode
71-82: LGTM!The AbstractLogStream methods are correctly implemented for managing stream activation state.
108-114: LGTM!Simple and correct getter/setter implementation for verbosity mode.
239-250: LGTM!Constructor and destructor are properly implemented, with consistent ownership semantics for managing log streams.
268-270: LGTM!Simple and correct implementation of standard output stream.
314-315: LGTM!Proper namespace closing.
| } | ||
|
|
||
| String Logger::getDateTime() { | ||
| static const uint32_t Space = 2; |
There was a problem hiding this comment.
Fix unused variable warning.
The variable Space is declared but unused since the DateTime implementation is commented out. Either implement the DateTime functionality or remove the unused variable to fix the compiler warning.
- static const uint32_t Space = 2;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static const uint32_t Space = 2; |
🧰 Tools
🪛 GitHub Actions: CMake
[warning] 253-253: Compiler warning: unused variable ‘Space’ [-Wunused-variable]
🤖 Prompt for AI Agents
In code/Common/Logger.cpp at line 253, the variable 'Space' is declared but
unused because the DateTime implementation is commented out. To fix the compiler
warning, either remove the declaration of 'Space' if the DateTime functionality
is not needed, or uncomment and implement the DateTime code that uses 'Space' so
the variable is utilized.
|
Summary by CodeRabbit
New Features
Improvements