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

[Mehdi Amini via llvm-commits]

> On Mar 22, 2016, at 8:36 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 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.

The point of unit testing is also to make sure no future change breaks a user facing expectation, so it is not totally useless.
But the reserve case should also include a test for a map that would already contains element, or a map initialized with some size, then reserved with an extra size.

> 
> ================
> 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)

Runtime is 5ms for this test.
Yeah that's annoying, we could do a bit more "white box testing", but it is not clear how robust it would be against future change of the internals of the map. What are you suggesting (beside adding a comment)?

Thanks,

Mehdi



> 
> ================
> 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
> 
> 
>