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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: unittests/ADT/DenseMapTest.cpp:372
@@ +371,3 @@
+  // DenseMap.
+  for (auto Size : ArrayRef<int>({1, 2, 48, 66})) {
+    DenseMap<int, CountCopyAndMove> Map(Size);
----------------
dblaikie wrote:
> I /think/ you can drop the ArrayRef<int>() here and just write:
> 
>   for (auto Size : {1, 2, 48, 66})
> 
> maybe?
> 
> Also, why 1 and 2? (maybe it's the right thing, just not clear to me - I take it your intent was that 1 is a boundary case, 2 is an "average" case below the minimum size, and 66 is an average case above the minimum size?)
1: boundary
2: small power of two
66: "random pick above 64 but not too big to not make the test too slow"

================
Comment at: unittests/ADT/DenseMapTest.cpp:371
@@ +370,3 @@
+  // Formula from DenseMap::getMinBucketToReserveForEntries()
+  const int ExpectedMaxInitialEntries = ExpectedInitialBucketCount * 3 / 4 - 1;
+
----------------
dblaikie wrote:
> I wouldn't expect to see this formula in the test case (since buckets are an implementation detail of the data structure) - maybe just hard code it to 47 as the max initial entries?
I'm not sure hardcoding the number would be better: the formula would still be in the test, but hidden in the relationship between the 64 and 47. And I'd probably add a comment to explain where the 47 comes from, and to do so I'd expose the formula.
I may also remove the `const int ExpectedInitialBucketCount`, but then I would have to reintroduce it in the comment to explain where the 47 comes...

If you're okay with me removing the default allocation of 64, I'll totally remove this test anyway in the next patch.


http://reviews.llvm.org/D18345