<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 24, 2016 at 11:46 AM, Mehdi AMINI via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">joker.eph added inline comments.<br>
<span class=""><br>
================<br>
Comment at: unittests/ADT/DenseMapTest.cpp:372<br>
@@ +371,3 @@<br>
+  // DenseMap.<br>
+  for (auto Size : ArrayRef<int>({1, 2, 48, 66})) {<br>
+    DenseMap<int, CountCopyAndMove> Map(Size);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> I /think/ you can drop the ArrayRef<int>() here and just write:<br>
><br>
>   for (auto Size : {1, 2, 48, 66})<br>
><br>
> maybe?<br>
><br>
> 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?)<br>
</span>1: boundary<br>
2: small power of two<br>
66: "random pick above 64 but not too big to not make the test too slow"<br>
<span class=""><br>
================<br>
Comment at: unittests/ADT/DenseMapTest.cpp:371<br>
@@ +370,3 @@<br>
+  // Formula from DenseMap::getMinBucketToReserveForEntries()<br>
+  const int ExpectedMaxInitialEntries = ExpectedInitialBucketCount * 3 / 4 - 1;<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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?<br>
</span>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.<br>
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...<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If you're okay with me removing the default allocation of 64, I'll totally remove this test anyway in the next patch.<br></blockquote><div><br></div><div>Rightio - if someone asks for a smaller amount, it seems reasonable for us to give it to them, I think... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D18345" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18345</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>