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

@@ -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<const DerivedT *>(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
+      // First move the inline buckets into a temporary storage.
-      // entries into it.
+      typename AlignedCharArray<BucketT[InlineBuckets]>::union_type
-      // FIXME: This is wasteful, we don't need the inline buffer here, and we
+        TmpStorage;
-      // certainly don't need to initialize it to empty.
+      BucketT *TmpBegin = reinterpret_cast<BucketT *>(TmpStorage.buffer);
-      SmallDenseMap TmpMap;
+      BucketT *TmpEnd = TmpBegin;
-      TmpMap.Small = false;
-      new (TmpMap.getLargeRep()) LargeRep(allocateBuckets(AtLeast));
-      TmpMap.moveFromOldBuckets(getInlineBuckets(),
-                                getInlineBuckets()+InlineBuckets);
 
-      // Now steal the innards back into this map, and arrange for the
+      // Loop over the buckets, moving non-empty, non-tombstones into the
-      // temporary map to be cleanly torn down.
+      // temporary storage. Have the loop move the TmpEnd forward as it goes.
-      assert(NumEntries == TmpMap.NumEntries);
+      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(allocateBuckets(AtLeast));
-      new (getLargeRep()) LargeRep(llvm_move(*TmpMap.getLargeRep()));
+      this->moveFromOldBuckets(TmpBegin, TmpEnd);
-      TmpMap.getLargeRep()->~LargeRep();
-      TmpMap.Small = true;
       return;
     }
 

unittests/ADT/DenseMapTest.cpp

@@ -10,6 +10,7 @@
 #include "gtest/gtest.h"
 #include "llvm/ADT/DenseMap.h"
 #include <map>
+#include <set>
 
 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<CtorTester *> 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 *> 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<T>::dummy_value_ptr = 0;
 // Register these types for testing.
 typedef ::testing::Types<DenseMap<uint32_t, uint32_t>,
                          DenseMap<uint32_t *, uint32_t *>,
+                         DenseMap<CtorTester, CtorTester, CtorTesterMapInfo>,
                          SmallDenseMap<uint32_t, uint32_t>,
-                         SmallDenseMap<uint32_t *, uint32_t *>
+                         SmallDenseMap<uint32_t *, uint32_t *>,
+                         SmallDenseMap<CtorTester, CtorTester, 4,
+                                       CtorTesterMapInfo>
                          > DenseMapTestTypes;
 TYPED_TEST_CASE(DenseMapTest, DenseMapTestTypes);