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

[David Blaikie via llvm-commits]

On Thu, Mar 24, 2016 at 11:46 AM, Mehdi AMINI via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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

I wouldn't necessarily do that - but like I said, my head's all a bit topsy
turvy with all these changes, so I may not be making sense.


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

Rightio - if someone asks for a smaller amount, it seems reasonable for us
to give it to them, I think...


>
>
> http://reviews.llvm.org/D18345
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160324/5ad80db3/attachment.html>