New string concept #39
Conversation
WalkthroughThe updates introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Unit Test
participant TStringBase as TStringBase<char>
participant TStringView as TStringView<char>
Test->>TStringBase: Create default instance
Test->>TStringBase: set(ptr, size)
Test->>TStringBase: Check size(), c_str(), equality
Test->>TStringBase: reset()
Test->>TStringBase: Check size() == 0
Test->>TStringView: Create with ptr, len
Test->>TStringView: begin(), end()
Test->>TStringView: Iterate and check elements
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 2
🧹 Nitpick comments (2)
include/cppcore/Common/TStringView.h (1)
33-34: Add a description to the @brief tag.The documentation comment is missing a description. Please add a brief explanation of what
TStringViewdoes.Apply this diff to add documentation:
-/// @brief +/// @brief Provides a lightweight, non-owning view over a contiguous sequence of elements.include/cppcore/Common/TStringBase.h (1)
145-152: Consider the risk of hash collisions in equality comparison.The equality operator relies solely on hash comparison after size check. While hash collisions are rare, they can occur. Consider adding actual data comparison for critical use cases or document this behavior.
For complete correctness, consider comparing the actual string data:
template <class T> inline bool TStringBase<T>::operator == (const TStringBase<T> &rhs) const { if (rhs.mSize != mSize) { return false; } - - return mHashId == rhs.mHashId; + if (mHashId != rhs.mHashId) { + return false; + } + + // Compare actual data to handle hash collisions + const T* lhsData = c_str(); + const T* rhsData = rhs.c_str(); + return memcmp(lhsData, rhsData, mSize * sizeof(T)) == 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(2 hunks)include/cppcore/CPPCoreCommon.h(2 hunks)include/cppcore/Common/Sort.h(1 hunks)include/cppcore/Common/TStringBase.h(2 hunks)include/cppcore/Common/TStringView.h(1 hunks)include/cppcore/Memory/MemUtils.h(0 hunks)test/common/TStringBaseTest.cpp(1 hunks)test/common/TStringViewTest.cpp(1 hunks)test/container/THashMapTest.cpp(1 hunks)test/container/TQueueTest.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- test/container/TQueueTest.cpp
- include/cppcore/Memory/MemUtils.h
🔇 Additional comments (12)
include/cppcore/Common/Sort.h (1)
179-181: Documentation spacing tweak LGTM
The alignment change after@tparamimproves readability; no functional impact.CMakeLists.txt (2)
79-79: LGTM: Build system integration for new string view class.The addition of
TStringView.hto the common source group properly integrates the new string view functionality into the build system.
141-142: LGTM: Test integration for new string classes.The addition of test files for both
TStringViewandTStringBaseensures proper test coverage for the new string functionality.test/container/THashMapTest.cpp (1)
33-34: LGTM: Consistent formatting improvements.The changes consistently use brace initialization and improve spacing in macros, enhancing code style consistency without affecting functionality.
Also applies to: 42-42, 45-45, 47-49, 53-53, 58-58
include/cppcore/CPPCoreCommon.h (2)
48-61: LGTM: Macro formatting improvements.The formatting improvements to DLL export/import macros and stack allocation macros enhance readability while maintaining functionality.
110-111: LGTM: Well-designed hash identifier type.The introduction of
HashIdas auint64_talias provides a standardized type for hash identifiers across the codebase, supporting the new string functionality with consistent hash-based operations.test/common/TStringViewTest.cpp (2)
33-42: LGTM: Well-structured construction test.The test correctly validates
TStringViewconstruction and iterator functionality by checking that the distance between begin and end iterators matches the expected size.
44-52: LGTM: Comprehensive iteration test.The test thoroughly validates iterator functionality by comparing each character accessed through the
TStringViewwith the original string data, ensuring correct element access behavior.test/common/TStringBaseTest.cpp (2)
33-38: LGTM: Effective equality comparison test.The test correctly validates
TStringBaseconstruction and equality comparison by creating two strings with the same content through different methods and verifying they compare as equal.
40-49: LGTM: Comprehensive functionality test.The test thoroughly validates the core
TStringBaseoperations including default initialization, setting data, size/capacity queries, content verification, and reset functionality. The default capacity of 256 appears reasonable for a string implementation.include/cppcore/Common/TStringView.h (1)
83-86: Verify handling of null pointer arithmetic.The
end()method performs pointer arithmetic without checking ifmPtris null. While null + 0 is well-defined, null + non-zero is undefined behavior.Consider adding a check or documenting that the constructor should ensure
mPtris non-null whenmLen > 0.include/cppcore/Common/TStringBase.h (1)
128-142: ```shell
#!/bin/bashDisplay constructors, set, copyFrom, and c_str implementations
sed -n '1,300p' include/cppcore/Common/TStringBase.h
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(6 hunks)include/cppcore/Common/TStringBase.h(2 hunks)include/cppcore/Common/TStringView.h(1 hunks)test/common/HashTest.cpp(0 hunks)test/common/TOptionalTest.cpp(1 hunks)test/common/TStringViewTest.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- test/common/HashTest.cpp
✅ Files skipped from review due to trivial changes (2)
- test/common/TOptionalTest.cpp
- include/cppcore/Common/TStringView.h
🚧 Files skipped from review as they are similar to previous changes (2)
- test/common/TStringViewTest.cpp
- CMakeLists.txt
🔇 Additional comments (4)
include/cppcore/Common/TStringBase.h (4)
104-106: LGTM!The capacity method implementation is correct and straightforward.
109-115: LGTM!The c_str() method correctly returns the appropriate buffer pointer based on whether dynamic allocation is used.
81-83: LGTM!The destructor correctly delegates cleanup to the clear() method, following good RAII practices.
99-101: LGTM!The size method implementation is correct and straightforward.
include/cppcore/Common/TStringBase.h
Outdated
| inline void TStringBase<T>::clear() { | ||
| if (mStringBuffer != nullptr) { | ||
| delete [] mStringBuffer; | ||
| mStringBuffer = nullptr; | ||
| mCapacity = InitSize; | ||
| } | ||
| mSize = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset hash ID when clearing string data.
The clear() method should also reset the hash ID to maintain consistency.
inline void TStringBase<T>::clear() {
if (mStringBuffer != nullptr) {
delete [] mStringBuffer;
mStringBuffer = nullptr;
mCapacity = InitSize;
}
mSize = 0;
+ mHashId = 0;
}📝 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>::clear() { | |
| if (mStringBuffer != nullptr) { | |
| delete [] mStringBuffer; | |
| mStringBuffer = nullptr; | |
| mCapacity = InitSize; | |
| } | |
| mSize = 0; | |
| } | |
| inline void TStringBase<T>::clear() { | |
| if (mStringBuffer != nullptr) { | |
| delete [] mStringBuffer; | |
| mStringBuffer = nullptr; | |
| mCapacity = InitSize; | |
| } | |
| mSize = 0; | |
| mHashId = 0; | |
| } |
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h around lines 118 to 125, the clear()
method deletes the string buffer and resets size and capacity but does not reset
the hash ID. Modify the clear() method to also reset the hash ID member variable
to its initial state to ensure consistency after clearing the string data.
| 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) { | ||
| base.mStringBuffer = new T[size]; | ||
| base.mCapacity = size; | ||
| targetPtr = base.mStringBuffer; | ||
| } | ||
| memcpy(targetPtr, ptr, size); | ||
| base.mSize = size; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Address unsafe memory operations and missing null termination.
The copyFrom method has several critical issues:
- Using
memcpyfor potentially non-trivial types is unsafe - No null termination for C-style strings
- Missing size validation
inline void TStringBase<T>::copyFrom(TStringBase<T> &base, const T *ptr, size_t size) {
+ static_assert(std::is_trivially_copyable_v<T>, "T must be trivially copyable");
if (ptr != nullptr) {
T *targetPtr = base.mBuffer;
if (size > 0) {
- if (size > base.mCapacity) {
- base.mStringBuffer = new T[size];
- base.mCapacity = size;
+ size_t requiredCapacity = size + 1; // +1 for null terminator
+ if (requiredCapacity > base.mCapacity) {
+ base.mStringBuffer = new T[requiredCapacity];
+ base.mCapacity = requiredCapacity;
targetPtr = base.mStringBuffer;
}
memcpy(targetPtr, ptr, size);
+ targetPtr[size] = T{}; // Null termination
base.mSize = size;
}
}
}📝 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) { | |
| base.mStringBuffer = new T[size]; | |
| base.mCapacity = size; | |
| targetPtr = base.mStringBuffer; | |
| } | |
| memcpy(targetPtr, ptr, size); | |
| base.mSize = size; | |
| } | |
| } | |
| } | |
| inline void TStringBase<T>::copyFrom(TStringBase<T> &base, const T *ptr, size_t size) { | |
| static_assert(std::is_trivially_copyable_v<T>, "T must be trivially copyable"); | |
| if (ptr != nullptr) { | |
| T *targetPtr = base.mBuffer; | |
| if (size > 0) { | |
| size_t requiredCapacity = size + 1; // +1 for null terminator | |
| if (requiredCapacity > base.mCapacity) { | |
| base.mStringBuffer = new T[requiredCapacity]; | |
| base.mCapacity = requiredCapacity; | |
| targetPtr = base.mStringBuffer; | |
| } | |
| memcpy(targetPtr, ptr, size); | |
| targetPtr[size] = T{}; // Null termination | |
| base.mSize = size; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h around lines 128 to 142, the copyFrom
method uses memcpy which is unsafe for non-trivial types, lacks null termination
for C-style strings, and does not validate the size properly. Replace memcpy
with element-wise copy or std::copy to safely handle non-trivial types, ensure
the copied string is null-terminated by adding a terminator if applicable, and
add checks to validate that size does not exceed capacity or other logical
constraints before copying.
| inline TStringBase<T>::TStringBase(const T *pPtr, size_t size) { | ||
| copyFrom(*this, pPtr, size); | ||
| mHashId = THash<HashId>::toHash(pPtr, mSize); | ||
| } |
There was a problem hiding this comment.
Fix hash calculation in constructor.
The hash is calculated using the input pointer pPtr and mSize before copyFrom completes. This could result in incorrect hash values since mSize may not be properly set yet, and the hash should be calculated from the stored data, not the input data.
inline TStringBase<T>::TStringBase(const T *pPtr, size_t size) {
copyFrom(*this, pPtr, size);
- mHashId = THash<HashId>::toHash(pPtr, mSize);
+ mHashId = THash<HashId>::toHash(c_str(), mSize);
}📝 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 TStringBase<T>::TStringBase(const T *pPtr, size_t size) { | |
| copyFrom(*this, pPtr, size); | |
| mHashId = THash<HashId>::toHash(pPtr, mSize); | |
| } | |
| inline TStringBase<T>::TStringBase(const T *pPtr, size_t size) { | |
| copyFrom(*this, pPtr, size); | |
| mHashId = THash<HashId>::toHash(c_str(), mSize); | |
| } |
🤖 Prompt for AI Agents
In include/cppcore/Common/TStringBase.h around lines 75 to 78, the constructor
calculates the hash using pPtr and mSize before copyFrom sets mSize and copies
the data. To fix this, move the hash calculation to after the copyFrom call and
use the internal data pointer and mSize to compute the hash, ensuring the hash
reflects the stored data correctly.
|
Summary by CodeRabbit
New Features
Refactor
Style
Tests
Chores