[PATCH] D18345: Fix DenseMap::reserve(): the formula was wrong

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: unittests/ADT/DenseMapTest.cpp:367
@@ +366,3 @@
+// buckets to insert N items without increasing allocation size.
+TEST(DenseMapCustomTest, InitialSizeTest) {
+  for (unsigned Size = 1; Size <= 1024; ++Size) {
----------------
If both the ctor and reserve now use common code, might not be necessary to test all parts of that code through both access points.

================
Comment at: unittests/ADT/DenseMapTest.cpp:368
@@ +367,3 @@
+TEST(DenseMapCustomTest, InitialSizeTest) {
+  for (unsigned Size = 1; Size <= 1024; ++Size) {
+    DenseMap<unsigned, CountCopyAndMove> Map(Size);
----------------
Do we need to test it for all these values? Or could we be a bit more specific? (I worry about shotgun testing like this - hard to maintain (because it's not clear what cases it's testing) & can add time to our test runs, make them harder to debug (because they're doing so much unintended/unnecessary work), etc)

================
Comment at: unittests/ADT/DenseMapTest.cpp:377
@@ +376,3 @@
+    EXPECT_EQ(Size * 2, CountCopyAndMove::Move);
+    EXPECT_EQ((unsigned)0, CountCopyAndMove::Copy);
+  }
----------------
You could use '0u' for the unsigned 0 literal rather than a cast

================
Comment at: unittests/ADT/DenseMapTest.cpp:384
@@ +383,3 @@
+TEST(DenseMapCustomTest, ReserveTest) {
+  for (unsigned Size = 1; Size <= 1024; ++Size) {
+    DenseMap<unsigned, CountCopyAndMove> Map;
----------------
Same question here


http://reviews.llvm.org/D18345