From ea5804adcabf6433f80fa58ba2267cdf87b53dc5 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sun, 17 Jun 2012 10:33:51 +0000 Subject: [PATCH] Add tests for *DenesMap for both key and value types' construction and destruction and fix a bug in SmallDenseMap they caught. This is kind of a poor-man's version of the testing that just adds the addresses to a set on construction and removes them on destruction. We check that double construction and double destruction don't occur. Amusingly enough, this is enough to catch a lot of SmallDenseMap issues because we spend a lot of time with fixed stable addresses in the inline buffer. The SmallDenseMap bug fix included makes grow() not double-destroy in some cases. It also fixes a FIXME there, the code was pretty crappy. We now don't have any wasted initialization, but we do move the entries in inline bucket array an extra time. It's probably a better tradeoff, and is much easier to get correct. llvm-svn: 158639 --- include/llvm/ADT/DenseMap.h | 44 +++++++++++++++++++------------ unittests/ADT/DenseMapTest.cpp | 47 +++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/include/llvm/ADT/DenseMap.h b/include/llvm/ADT/DenseMap.h index 8c5793853a4..dcae0de19d4 100644 --- a/include/llvm/ADT/DenseMap.h +++ b/include/llvm/ADT/DenseMap.h @@ -310,7 +310,6 @@ protected: std::swap(getNumTombstones(), RHS.getNumTombstones()); } -private: static unsigned getHashValue(const KeyT &Val) { return KeyInfoT::getHashValue(Val); } @@ -325,6 +324,7 @@ private: return KeyInfoT::getTombstoneKey(); } +private: unsigned getNumEntries() const { return static_cast(this)->getNumEntries(); } @@ -803,24 +803,34 @@ public: if (AtLeast <= InlineBuckets) return; // Nothing to do. - // First grow an allocated bucket array in another map and move our - // entries into it. - // FIXME: This is wasteful, we don't need the inline buffer here, and we - // certainly don't need to initialize it to empty. - SmallDenseMap TmpMap; - TmpMap.Small = false; - new (TmpMap.getLargeRep()) LargeRep(allocateBuckets(AtLeast)); - TmpMap.moveFromOldBuckets(getInlineBuckets(), - getInlineBuckets()+InlineBuckets); + // First move the inline buckets into a temporary storage. + typename AlignedCharArray::union_type + TmpStorage; + BucketT *TmpBegin = reinterpret_cast(TmpStorage.buffer); + BucketT *TmpEnd = TmpBegin; - // Now steal the innards back into this map, and arrange for the - // temporary map to be cleanly torn down. - assert(NumEntries == TmpMap.NumEntries); + // Loop over the buckets, moving non-empty, non-tombstones into the + // temporary storage. Have the loop move the TmpEnd forward as it goes. + const KeyT EmptyKey = this->getEmptyKey(); + const KeyT TombstoneKey = this->getTombstoneKey(); + for (BucketT *P = getBuckets(), *E = P + InlineBuckets; P != E; ++P) { + if (!KeyInfoT::isEqual(P->first, EmptyKey) && + !KeyInfoT::isEqual(P->first, TombstoneKey)) { + assert((TmpEnd - TmpBegin) < InlineBuckets && + "Too many inline buckets!"); + new (&TmpEnd->first) KeyT(llvm_move(P->first)); + new (&TmpEnd->second) ValueT(llvm_move(P->second)); + ++TmpEnd; + P->second.~ValueT(); + } + P->first.~KeyT(); + } + + // Now make this map use the large rep, and move all the entries back + // into it. Small = false; - NumTombstones = llvm_move(TmpMap.NumTombstones); - new (getLargeRep()) LargeRep(llvm_move(*TmpMap.getLargeRep())); - TmpMap.getLargeRep()->~LargeRep(); - TmpMap.Small = true; + new (getLargeRep()) LargeRep(allocateBuckets(AtLeast)); + this->moveFromOldBuckets(TmpBegin, TmpEnd); return; } diff --git a/unittests/ADT/DenseMapTest.cpp b/unittests/ADT/DenseMapTest.cpp index d61f991bd55..bd9a3f24ec3 100644 --- a/unittests/ADT/DenseMapTest.cpp +++ b/unittests/ADT/DenseMapTest.cpp @@ -10,6 +10,7 @@ #include "gtest/gtest.h" #include "llvm/ADT/DenseMap.h" #include +#include using namespace llvm; @@ -29,6 +30,47 @@ uint32_t *getTestValue(int i, uint32_t **) { return &dummy_arr1[i]; } +/// \brief A test class that tries to check that construction and destruction +/// occur correctly. +class CtorTester { + static std::set Constructed; + int Value; + +public: + explicit CtorTester(int Value = 0) : Value(Value) { + EXPECT_TRUE(Constructed.insert(this).second); + } + CtorTester(uint32_t Value) : Value(Value) { + EXPECT_TRUE(Constructed.insert(this).second); + } + CtorTester(const CtorTester &Arg) : Value(Arg.Value) { + EXPECT_TRUE(Constructed.insert(this).second); + } + ~CtorTester() { + EXPECT_EQ(1u, Constructed.erase(this)); + } + operator uint32_t() const { return Value; } + + int getValue() const { return Value; } + bool operator==(const CtorTester &RHS) const { return Value == RHS.Value; } +}; + +std::set CtorTester::Constructed; + +struct CtorTesterMapInfo { + static inline CtorTester getEmptyKey() { return CtorTester(-1); } + static inline CtorTester getTombstoneKey() { return CtorTester(-2); } + static unsigned getHashValue(const CtorTester &Val) { + return Val.getValue() * 37u; + } + static bool isEqual(const CtorTester &LHS, const CtorTester &RHS) { + return LHS == RHS; + } +}; + +CtorTester getTestKey(int i, CtorTester *) { return CtorTester(i); } +CtorTester getTestValue(int i, CtorTester *) { return CtorTester(42 + i); } + // Test fixture, with helper functions implemented by forwarding to global // function overloads selected by component types of the type parameter. This // allows all of the map implementations to be tested with shared @@ -57,8 +99,11 @@ typename T::mapped_type *const DenseMapTest::dummy_value_ptr = 0; // Register these types for testing. typedef ::testing::Types, DenseMap, + DenseMap, SmallDenseMap, - SmallDenseMap + SmallDenseMap, + SmallDenseMap > DenseMapTestTypes; TYPED_TEST_CASE(DenseMapTest, DenseMapTestTypes);