Skip to content

Commit

Permalink
Merge pull request #11 from SergeyMakeev/fix-bug-with-value-dtor-and-…
Browse files Browse the repository at this point in the history 
…ctor

1. Fix the issue with value's destructors not being called sometimes
2. Add more tests to catch similar issues in the future
  • Loading branch information
@SergeyMakeev
SergeyMakeev committed Feb 5, 2024
2 parents 7d001e9 + 371e9eb commit 711382a
Show file tree
Hide file tree
Showing 2 changed files with 194 additions and 22 deletions.
51 changes: 35 additions & 16 deletions ExcaliburHash/ExcaliburHash.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,15 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
inline void moveFrom(HashTable&& other)
{
// note: the current hash table is supposed to be destroyed/non-initialized

if (!other.isUsingInlineStorage())
{
// if not using inline storage than it's a simple pointer swap
// if we are not using inline storage than it's a simple pointer swap
constructInline(TKeyInfo::getEmpty());
m_storage = other.m_storage;
m_numBuckets = other.m_numBuckets;
m_numElements = other.m_numElements;
other.m_storage = nullptr;
// don't need to zero rest of the members because dtor doesn't use them
// other.m_numBuckets = 0;
// other.m_numElements = 0;
}
Expand All @@ -258,7 +258,8 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
m_storage = inlineItems;
m_numBuckets = other.m_numBuckets;
m_numElements = other.m_numElements;
other.m_storage = nullptr;
// note: other's online items will be destroyed automatically when its dtor called
// other.m_storage = nullptr;
// other.m_numBuckets = 0;
// other.m_numElements = 0;
}
Expand Down Expand Up @@ -306,7 +307,7 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
return inlineItems;
}

inline TItem* moveInline(TItem* from)
inline TItem* moveInline(TItem* otherInlineItems)
{
TItem* inlineItems = reinterpret_cast<TItem*>(&m_inlineStorage);

Expand All @@ -316,21 +317,18 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
for (unsigned i = 0; i < kNumInlineItems; i++)
{
TItem* inlineItem = (inlineItems + i);
TItem& otherInlineItem = from[i];
const bool hasValidValue = otherInlineItem.isValid();
construct<TItem>((inlineItems + i), std::move(*otherInlineItem.key()));
TItem* otherInlineItem = (otherInlineItems + i);
const bool hasValidValue = otherInlineItem->isValid();
// move construct key
construct<TItem>((inlineItems + i), std::move(*otherInlineItem->key()));

// move inline storage value (if any)
if (hasValidValue)
{
TValue* value = inlineItem->value();
TValue* otherValue = otherInlineItem.value();
TValue* otherValue = otherInlineItem->value();
// move construct value
construct<TValue>(value, std::move(*otherValue));

if constexpr (!std::is_trivially_destructible<TValue>::value)
{
destruct(otherValue);
}
}
}
}
Expand All @@ -339,7 +337,8 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
// move only keys
for (unsigned i = 0; i < kNumInlineItems; i++)
{
construct<TItem>((inlineItems + i), std::move(*from[i].key()));
// move construct key
construct<TItem>((inlineItems + i), std::move(*(otherInlineItems + i)->key()));
}
}

Expand Down Expand Up @@ -386,6 +385,16 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
TItem* const endItem = item + numBuckets;
for (; item != endItem; item++)
{
// destroy value if need
if constexpr (!std::is_trivially_destructible<TValue>::value)
{
if (item->isValid())
{
destruct(item->value());
}
}

// note: this won't automatically call value dtors!
destruct(item);
}
}
Expand Down Expand Up @@ -721,7 +730,15 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
{
emplaceToExisting(numBucketsNew, std::move(*item->key()));
}

// destroy old value if need
if constexpr ((!std::is_trivially_destructible<TValue>::value) && (has_values::value))
{
destruct(item->value());
}
}

// note: this won't automatically call value dtors!
destruct(item);
}
}
Expand Down Expand Up @@ -989,9 +1006,11 @@ template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename
};

// hashmap declaration
template <typename TKey, typename TValue> using HashMap = HashTable<TKey, TValue, 1, KeyInfo<TKey>>;
template <typename TKey, typename TValue, unsigned kNumInlineItems = 1, typename TKeyInfo = KeyInfo<TKey>>
using HashMap = HashTable<TKey, TValue, kNumInlineItems, TKeyInfo>;

// hashset declaration
template <typename TKey> using HashSet = HashTable<TKey, std::nullptr_t, 1, KeyInfo<TKey>>;
template <typename TKey, unsigned kNumInlineItems = 1, typename TKeyInfo = KeyInfo<TKey>>
using HashSet = HashTable<TKey, std::nullptr_t, kNumInlineItems, TKeyInfo>;

} // namespace Excalibur
165 changes: 159 additions & 6 deletions ExcaliburHashTest05.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,165 @@ TEST(SmFlatHashMap, InlineStorageTest01)
}
}

static int ctorCallCount = 0;
static int dtorCallCount = 0;
static int assignCallCount = 0;

struct ComplexVal
{
ComplexVal() = delete;
ComplexVal(const ComplexVal& other) = delete;
ComplexVal& operator=(const ComplexVal& other) noexcept = delete;

ComplexVal(uint32_t _v) noexcept
: v(_v)
, status(Status::Constructed)
{
ctorCallCount++;
}

ComplexVal(ComplexVal&& other) noexcept
: v(other.v)
, status(Status::MoveConstructed)
{
EXPECT_NE(other.status, Status::Destructed);
ctorCallCount++;
other.status = Status::Moved;
}

~ComplexVal() noexcept
{
EXPECT_NE(status, Status::Destructed);
status = Status::Destructed;
dtorCallCount++;
}

ComplexVal& operator=(ComplexVal&& other) noexcept
{
status = Status::MoveAssigned;
other.status = Status::Moved;

v = other.v;
assignCallCount++;
return *this;
}

uint32_t v;
enum class Status
{
Constructed = 0,
MoveConstructed = 1,
MoveAssigned = 2,
Destructed = 3,
Moved = 4,
};
Status status;
};

template <typename TFrom, typename TTo> void doMoveAssignmentTest(TFrom& hm1, TTo& hm2, size_t numValuesToInsert)
{
// insert values
for (size_t i = 0; i < numValuesToInsert; i++)
{
hm1.emplace(int(i), uint32_t(i + 13));
}

{
EXPECT_TRUE(hm1.has(0));
EXPECT_TRUE(hm1.has(1));
EXPECT_TRUE(hm1.has(2));

auto it1 = hm1.find(0);
auto it2 = hm1.find(1);
auto it3 = hm1.find(2);

EXPECT_NE(it1, hm1.end());
EXPECT_NE(it2, hm1.end());
EXPECT_NE(it3, hm1.end());

const int& key1 = it1->first;
const ComplexVal& value1 = it1->second;

const int& key2 = it2->first;
const ComplexVal& value2 = it2->second;

const int& key3 = it3->first;
const ComplexVal& value3 = it3->second;

EXPECT_EQ(key1, 0);
EXPECT_EQ(key2, 1);
EXPECT_EQ(key3, 2);

EXPECT_EQ(value1.v, uint32_t(13));
EXPECT_EQ(value2.v, uint32_t(14));
EXPECT_EQ(value3.v, uint32_t(15));
}

// move assign to other hash map
hm2 = std::move(hm1);

{
EXPECT_TRUE(hm2.has(0));
EXPECT_TRUE(hm2.has(1));
EXPECT_TRUE(hm2.has(2));

auto it1 = hm2.find(0);
auto it2 = hm2.find(1);
auto it3 = hm2.find(2);

EXPECT_NE(it1, hm2.end());
EXPECT_NE(it2, hm2.end());
EXPECT_NE(it3, hm2.end());

const int& key1 = it1->first;
const ComplexVal& value1 = it1->second;

const int& key2 = it2->first;
const ComplexVal& value2 = it2->second;

const int& key3 = it3->first;
const ComplexVal& value3 = it3->second;

EXPECT_EQ(key1, 0);
EXPECT_EQ(key2, 1);
EXPECT_EQ(key3, 2);

EXPECT_EQ(value1.v, uint32_t(13));
EXPECT_EQ(value2.v, uint32_t(14));
EXPECT_EQ(value3.v, uint32_t(15));
}
}

TEST(SmFlatHashMap, InlineStorageTest02)
{
{
// move inline -> inline
ctorCallCount = 0;
dtorCallCount = 0;
assignCallCount = 0;
{
Excalibur::HashMap<int, ComplexVal, 8> hm1;
Excalibur::HashMap<int, ComplexVal, 8> hm2;

doMoveAssignmentTest(hm1, hm2, 3);
}
EXPECT_EQ(ctorCallCount, dtorCallCount);
}

{
// move heap -> heap
ctorCallCount = 0;
dtorCallCount = 0;
assignCallCount = 0;
{
Excalibur::HashMap<int, ComplexVal, 1> hm1;
Excalibur::HashMap<int, ComplexVal, 1> hm2;

doMoveAssignmentTest(hm1, hm2, 100);
}
EXPECT_EQ(ctorCallCount, dtorCallCount);
}
}

TEST(SmFlatHashMap, AliasNameTest)
{
Expand Down Expand Up @@ -101,10 +260,4 @@ TEST(SmFlatHashMap, AliasNameTest)
EXPECT_TRUE(hs.has(2));
EXPECT_FALSE(hs.has(3));
}






}

0 comments on commit 711382a

Please sign in to comment.