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

[David Blaikie via llvm-commits]

dblaikie 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);
----------------
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?)

================
Comment at: unittests/ADT/DenseMapTest.cpp:391
@@ +390,3 @@
+  std::vector<std::pair<int, CountCopyAndMove>> Values;
+  // The size is a random value greater than 64 (hardcoded DenseMap min init)
+  const int Count = 65;
----------------
It's not really random though. Perhaps "Ensure that when initializing with a range larger than the default minimum, reallocation doesn't occur"? (maybe that could be the test header comment? It's specifically not just about using an iterator range, but a range larger than the default number of buckets, right?)

Also, do we do this even when the iterators aren't random access? (I'd be suprrised if we paid the extra O(n) scan to find the size of the range first) Not to suggest you need to test that too - but maybe worth a comment that this test is only about random access iterators, where computing the delta between two iterators is O(1)

Though this isn't quite the test I had in mind/was suggesting (perhaps it wasn't your intent to implement the test I was suggesting - sorry for the confusion, if that's the case)

I was suggesting demonstrating that the default minimum size is what we're relying on in other tests: default construct a DenseMap, add up to the default minimum, check that nothing got reallocated/moved. Then add one more element and demonstrate that reallocation/moving did happen. That way, if someone changes the default minimum that test will fail - and we can include some breadcrumbs about how to update other tests. "WARNING: IF YOU UPDATE THIS TEST, UPDATE SOME OTHER TESTS TOO" or something


http://reviews.llvm.org/D18345