Skip to content

Hash: Move to template #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Hash: Move to template #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e40e50a
Hash: Move to template
kullingk Oct 23, 2024
ec0a4b6
Update include/cppcore/Container/THashMap.h
kimkulling Oct 23, 2024
63857df
Update Hash.h
kimkulling Oct 23, 2024
e788628
Update Hash.h
kimkulling Oct 23, 2024
d3ef0a1
Update Hash.h
kimkulling Oct 23, 2024
f60b2d7
Update Hash.h
kimkulling Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 30 additions & 26 deletions include/cppcore/Common/Hash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,79 +27,81 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
namespace cppcore {

//-------------------------------------------------------------------------------------------------
/// @class Hash
/// @class THash
/// @ingroup CPPCore
///
/// @brief This class is used to calculate the hash value for a given integer or character buffer.
//-------------------------------------------------------------------------------------------------
class Hash {
template<class T>
class THash {
public:
/// @brief The default class constructor.
Hash();
THash();

/// @brief The class constructor with a given hash value.
/// @param hash [in] An integer value to compute the hash from.
explicit Hash(unsigned int hash);
explicit THash(T hash);

/// @brief The class constructor with a given char buffer.
/// @param value [in] A character buffer to compute the hash from.
/// @param base [in] The table base.
explicit Hash(const char *buffer, unsigned int base);
explicit THash(const char *buffer, T base);

/// @brief The class constructor with a given unsigned int value.
/// @param value [in] An unsigned int value to compute the hash from.
/// @param base [in] The table base.
explicit Hash(unsigned int value, unsigned int base);
explicit THash(T value, T base);

/// @brief The class destructor.
~Hash();
~THash() = default;

/// @brief Computes the hash value for a given character buffer.
/// @param buffer [in] The buffer.
/// @param base [in] The table base.
/// @return The hash value.
static unsigned int toHash(const char *buffer, unsigned int base);
static T toHash(const char *buffer, T base);

/// @brief Computes the hash value for a given unsigned int value.
/// @param buffer [in] The unsigned int value.
/// @param base [in] The table base.
/// @return The hash value.
static unsigned int toHash(unsigned int value, unsigned int base);
static T toHash(T value, T base);

/// brief Returns the stored hash value.
/// @return The hash value.
unsigned int hashValue() const;
T hashValue() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


private:
unsigned int m_hash;
T m_hash;
};

inline Hash::Hash() :
template <class T>
inline THash<T>::THash() :
m_hash(0) {
// empty
}

inline Hash::Hash(unsigned int hash) :
template <class T>
inline THash<T>::THash(T hash) :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

m_hash(hash) {
// empty
}

inline Hash::Hash(const char *buffer, unsigned int base) :
m_hash(Hash::toHash(buffer, base)) {
template <class T>
inline THash<T>::THash(const char *buffer, T base) :
m_hash(THash::toHash(buffer, base)) {
// empty
}

inline Hash::Hash(unsigned int value, unsigned int base) :
m_hash(Hash::toHash(value, base)) {
template <class T>
inline THash<T>::THash(T value, T base) :
m_hash(THash::toHash(value, base)) {
// empty
}

inline Hash::~Hash() {
// empty
}

inline unsigned int Hash::toHash(const char *buffer, unsigned int base) {
unsigned int hash(0);
template <class T>
inline T THash<T>::toHash(const char *buffer, T base) {
T hash = 0;
Comment on lines +103 to +104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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>

if (nullptr == buffer) {
return hash;
}
Expand All @@ -113,12 +115,14 @@ inline unsigned int Hash::toHash(const char *buffer, unsigned int base) {
return hash;
}

inline unsigned int Hash::toHash(unsigned int value, unsigned int base) {
const unsigned int hash(value % base);
template <class T>
inline T THash<T>::toHash(T value, T base) {
const T hash = value % base;
return hash;
}

inline unsigned int Hash::hashValue() const {
template <class T>
inline T THash<T>::hashValue() const {
return m_hash;
}

Expand Down
10 changes: 6 additions & 4 deletions include/cppcore/Container/THashMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ namespace cppcore {
template <class T, class U, class TAlloc = TDefaultAllocator<T>>
class THashMap {
public:
using Hash = THash<T>;

/// @brief The initial hash size.
static constexpr size_t InitSize = 1024;
/// @brief Marker for unset node keys.
Expand Down Expand Up @@ -176,7 +178,7 @@ inline void THashMap<T, U, TAlloc>::clear() {

template <class T, class U, class TAlloc>
inline void THashMap<T, U, TAlloc>::insert(const T &key, const U &value) {
const unsigned int hash = Hash::toHash(key, (unsigned int)m_buffersize);
const T hash = Hash::toHash(key, static_cast<T>(m_buffersize));
if (nullptr == m_buffer[hash]) {
Node *node = new Node;
node->m_key = key;
Expand All @@ -190,7 +192,7 @@ inline void THashMap<T, U, TAlloc>::insert(const T &key, const U &value) {

template <class T, class U, class TAlloc>
inline bool THashMap<T, U, TAlloc>::remove(const T &key) {
const unsigned int hash = Hash::toHash(key, (unsigned int)m_buffersize);
const T hash = Hash::toHash(key, (unsigned int)m_buffersize);
if (nullptr == m_buffer[hash]) {
return false;
}
Expand Down Expand Up @@ -220,7 +222,7 @@ inline bool THashMap<T, U, TAlloc>::hasKey(const T &key) const {
if (0 == m_buffersize) {
return false;
}
const unsigned int hash(Hash::toHash(key, (unsigned int)m_buffersize));
const T hash = THash<T>::toHash(key, (unsigned int)m_buffersize);
const Node *node = m_buffer[hash];
if (nullptr == node) {
return false;
Expand All @@ -245,7 +247,7 @@ inline bool THashMap<T, U, TAlloc>::hasKey(const T &key) const {

template <class T, class U, class TAlloc>
inline bool THashMap<T, U, TAlloc>::getValue(const T &key, U &value) const {
const size_t pos = Hash::toHash(key, (unsigned int)m_buffersize);
const size_t pos = Hash::toHash(key, (unsigned int) m_buffersize);
if (m_buffer[pos]->m_key == key) {
value = m_buffer[pos]->m_value;
return true;
Expand Down
29 changes: 16 additions & 13 deletions test/common/HashTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,67 +29,70 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
using namespace cppcore;

class HashTest : public testing::Test {
public:
using UiHash = THash<unsigned int>;

protected:
// empty
};

TEST_F( HashTest, CreateTest ) {
Hash myHash1;
UiHash myHash1;
EXPECT_EQ( myHash1.hashValue(), 0U );

Hash myHash2( 10U );
UiHash myHash2(10U);
EXPECT_EQ( myHash2.hashValue(), 10U );

Hash myHash3( "test", 7 );
UiHash myHash3("test", 7);
EXPECT_NE( myHash3.hashValue(), 0U );
}

TEST_F( HashTest, MakeStringHashTest ) {
static const unsigned int Base = 7;
Hash myHash_empty;
UiHash myHash_empty;
EXPECT_EQ( myHash_empty.hashValue(), 0U );

std::string value;
value = ( "huhu1" );
const unsigned int hash1 = Hash::toHash( value.c_str(), Base );
const unsigned int hash1 = UiHash::toHash(value.c_str(), Base);
EXPECT_NE( hash1, 0U );
EXPECT_LE( hash1, Base );

value = ( "huhu2" );
const unsigned int hash2 = Hash::toHash( value.c_str(), Base );
const unsigned int hash2 = UiHash::toHash(value.c_str(), Base);
EXPECT_NE( hash2, 0U );
EXPECT_LE( hash2, Base );

value = ( "huhu3" );
const unsigned int hash3 = Hash::toHash( value.c_str(), Base );
const unsigned int hash3 = UiHash::toHash(value.c_str(), Base);
EXPECT_NE( hash3, 0U );
EXPECT_LE( hash3, Base );

Hash myHash_inited( value.c_str(), Base );
UiHash myHash_inited(value.c_str(), Base);
EXPECT_EQ( myHash_inited.hashValue(), hash3 );
}

TEST_F( HashTest, MakeUIntHashTest ) {
static const unsigned int Base = 7U;
Hash myHash_empty;
UiHash myHash_empty;
EXPECT_EQ( myHash_empty.hashValue(), 0U );

unsigned int value = 17U;
const unsigned int hash1 = Hash::toHash( value, Base );
const unsigned int hash1 = UiHash::toHash(value, Base);

EXPECT_NE( hash1, 0U );
EXPECT_LE( hash1, Base );

value = 27U;
const unsigned int hash2 = Hash::toHash( value, Base );
const unsigned int hash2 = UiHash::toHash(value, Base);
EXPECT_NE( hash2, 0U );
EXPECT_LE( hash2, Base );

value = 37U;
const unsigned int hash3 = Hash::toHash( value, Base );
const unsigned int hash3 = UiHash::toHash(value, Base);
EXPECT_NE( hash3, 0U );
EXPECT_LE( hash3, Base );

Hash myHash_inited( value, Base );
UiHash myHash_inited(value, Base);
EXPECT_EQ( myHash_inited.hashValue(), hash3 );
}
Loading