Hash: Move to template #28
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant THash
participant THashMap
Client->>THash: Create THash<T>
THash->>THash: Process value of type T
THashMap->>THash: Insert value
THashMap->>THash: Retrieve value
THash-->>THashMap: Return hash value
THashMap-->>Client: Return result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 4
🧹 Outside diff range and nitpick comments (5)
test/common/HashTest.cpp (2)
52-72: Consider enhancing test robustness.The test effectively verifies basic string hashing functionality. Consider these improvements:
- Add assertions to verify that different strings produce different hash values (e.g.,
EXPECT_NE(hash1, hash2))- Document why 7 was chosen as the base value
TEST_F( HashTest, MakeStringHashTest ) { + // Using prime number 7 as base for better hash distribution static const unsigned int Base = 7; UiHash myHash_empty; EXPECT_EQ( myHash_empty.hashValue(), 0U ); std::string value; value = ( "huhu1" ); const unsigned int hash1 = UiHash::toHash(value.c_str(), Base); EXPECT_NE( hash1, 0U ); EXPECT_LE( hash1, Base ); value = ( "huhu2" ); const unsigned int hash2 = UiHash::toHash(value.c_str(), Base); EXPECT_NE( hash2, 0U ); EXPECT_LE( hash2, Base ); + EXPECT_NE( hash1, hash2 ); // Different strings should produce different hashes value = ( "huhu3" ); const unsigned int hash3 = UiHash::toHash(value.c_str(), Base); EXPECT_NE( hash3, 0U ); EXPECT_LE( hash3, Base ); + EXPECT_NE( hash2, hash3 ); // Different strings should produce different hashes
77-98: Consider enhancing test robustness (similar to string test).The test effectively verifies basic integer hashing functionality. Consider the same improvements as suggested for the string test:
- Add assertions to verify that different values produce different hash values
- Document the base value choice
TEST_F( HashTest, MakeUIntHashTest ) { + // Using prime number 7 as base for better hash distribution static const unsigned int Base = 7U; UiHash myHash_empty; EXPECT_EQ( myHash_empty.hashValue(), 0U ); unsigned int value = 17U; const unsigned int hash1 = UiHash::toHash(value, Base); EXPECT_NE( hash1, 0U ); EXPECT_LE( hash1, Base ); value = 27U; const unsigned int hash2 = UiHash::toHash(value, Base); EXPECT_NE( hash2, 0U ); EXPECT_LE( hash2, Base ); + EXPECT_NE( hash1, hash2 ); // Different values should produce different hashes value = 37U; const unsigned int hash3 = UiHash::toHash(value, Base); EXPECT_NE( hash3, 0U ); EXPECT_LE( hash3, Base ); + EXPECT_NE( hash2, hash3 ); // Different values should produce different hashesinclude/cppcore/Container/THashMap.h (3)
225-225: Use the Hash type alias for consistencyFor consistency with other methods, consider using the
Hashtype alias instead of directly usingTHash<T>.- const T hash = THash<T>::toHash(key, (unsigned int)m_buffersize); + const T hash = Hash::toHash(key, static_cast<T>(m_buffersize));
Line range hint
250-264: Fix potential null pointer dereferenceThe method doesn't check if
m_buffer[pos]is null before dereferencing it, which could lead to crashes.const size_t pos = Hash::toHash(key, (unsigned int) m_buffersize); + if (m_buffer[pos] == nullptr) { + return false; + } if (m_buffer[pos]->m_key == key) { value = m_buffer[pos]->m_value; return true; }
Line range hint
278-288: Fix critical pointer handling issues in operator[]There are several critical issues in this implementation:
Node nextis incorrectly declared as a value instead of a pointer- Missing null pointer check for
m_buffer[pos]- Returning a static dummy variable by reference could lead to undefined behavior if modified
static U dummy; const unsigned int pos = Hash::toHash(key, m_buffersize); + if (m_buffer[pos] == nullptr) { + return dummy; + } if (m_buffer[pos]->m_key == key) { return m_buffer[pos]->m_value; } - Node next = m_buffer[pos]->m_next; + Node* next = m_buffer[pos]->m_next; while (next->m_key != key) { next = next->m_next; if (nullptr == next) { return dummy; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- include/cppcore/Common/Hash.h (2 hunks)
- include/cppcore/Container/THashMap.h (5 hunks)
- test/common/HashTest.cpp (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
test/common/HashTest.cpp (3)
32-34: LGTM! Clear and well-placed type alias.The type alias
UiHashis appropriately defined and follows a clear naming convention that indicates both the underlying type and purpose.
40-47: LGTM! Comprehensive test coverage for hash creation.The test cases effectively verify all essential hash construction scenarios with appropriate assertions.
Line range hint
1-98: Verify complete migration to template-based implementation.Let's ensure all
Hashreferences have been converted to use the templated version.✅ Verification successful
Let me check the actual content of these files to better understand the usage patterns.
After analyzing the code, I can now provide a final response:
Template migration is complete and consistent
The codebase shows a proper and consistent migration to the templated
THashimplementation:
- The base template class
THash<T>is properly defined inHash.h- All usages in
THashMap.hcorrectly use the templated version through the aliasusing Hash = THash<T>- Test cases in
HashTest.cppuse the templated version throughUiHash = THash<unsigned int>- No legacy non-templated
Hashclass definitions existThe
Hash::references found are actuallyUiHash::orTHash::accessing static methods of the template class, which is the correct usage pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining non-templated Hash usage echo "Checking for any remaining non-templated Hash usage..." rg -l "class\s+Hash\s*[^<]" || echo "No direct Hash class definitions found" rg -l "Hash::" || echo "No direct Hash:: usage found" # Verify consistent template usage echo -e "\nVerifying template usage pattern..." rg -l "THash\s*<" || echo "No THash template usage found"Length of output: 479
Script:
#!/bin/bash # Check the content of Hash.h echo "=== Content of include/cppcore/Common/Hash.h ===" cat include/cppcore/Common/Hash.h # Check for Hash class definition and usage in THashMap.h echo -e "\n=== Content of include/cppcore/Container/THashMap.h ===" cat include/cppcore/Container/THashMap.h # Look for any Hash:: usage context echo -e "\n=== Hash:: usage contexts ===" rg "Hash::" -C 2Length of output: 18980
include/cppcore/Container/THashMap.h (1)
47-48: LGTM: Type alias improves code clarityThe addition of
using Hash = THash<T>aligns well with the template migration objective and improves code readability.include/cppcore/Common/Hash.h (1)
119-120: EnsureTis integral intoHash(T value, T base)Similar to the previous comment, the method
toHash(T value, T base)also uses the modulo operator (%), requiringTto be an integral type. The static assertion added earlier will cover this method as well.
|
|
||
| inline Hash::Hash(unsigned int hash) : | ||
| template <class T> | ||
| inline THash<T>::THash(T hash) : |
There was a problem hiding this comment.
Potential data loss when assigning T to unsigned int in constructor
In the constructor THash(T hash), the parameter hash of type T is assigned to m_hash of type unsigned int. If T has a larger range or different representation than unsigned int, this could cause data loss or unexpected behavior. Changing m_hash to type T would resolve this issue.
| inline T THash<T>::toHash(const char *buffer, T base) { | ||
| T hash = 0; |
There was a problem hiding this comment.
Constrain template parameter T to integral types
The method toHash(const char* buffer, T base) uses the modulo operator (%), which is only defined for integral types. To prevent misuse with non-integral types (e.g., floating-point types), add a static assertion to enforce that T is an integral type.
Add the following static assertion inside the class definition:
static_assert(std::is_integral<T>::value, "THash requires T to be an integral type.");Remember to include the <type_traits> header at the top of the file:
#include <type_traits>| /// brief Returns the stored hash value. | ||
| /// @return The hash value. | ||
| unsigned int hashValue() const; | ||
| T hashValue() const; |
There was a problem hiding this comment.
Inconsistent types between hashValue() return type and m_hash
The method hashValue() returns type T, but the member variable m_hash is declared as unsigned int. This could lead to implicit conversions and potential data loss if T differs from unsigned int. Consider changing m_hash to type T to ensure type consistency throughout the class.
Apply this diff to update the member variable type:
private:
- unsigned int m_hash;
+ T m_hash;Committable suggestion was skipped due to low confidence.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Summary by CodeRabbit
New Features
THash, that supports various data types.THashMapclass.Bug Fixes
THashMapmethods to ensure consistency with the new hash type.Tests
UiHashtype for hash-related operations.