<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 22, 2016, at 9:22 AM, Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Mar 22, 2016, at 8:59 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Mar 22, 2016 at 8:53 AM, Mehdi Amini via llvm-commits<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">> On Mar 22, 2016, at 8:36 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:<br class="">><br class="">> dblaikie added inline comments.<br class="">><br class="">> ================<br class="">> Comment at: unittests/ADT/DenseMapTest.cpp:367<br class="">> @@ +366,3 @@<br class="">> +// buckets to insert N items without increasing allocation size.<br class="">> +TEST(DenseMapCustomTest, InitialSizeTest) {<br class="">> + for (unsigned Size = 1; Size <= 1024; ++Size) {<br class="">> ----------------<br class="">> 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.<br class=""><br class=""></span>The point of unit testing is also to make sure no future change breaks a user facing expectation, so it is not totally useless.<br class="">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.<br class=""></blockquote><div class=""><br class=""></div><div class="">Sure - it seems like the reserve case would test a superset of the ctor case. I'd be inclined just to test one or two cases in the ctor, then implement it in such a way that it's mostly just "calls reserve".<br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">Note that the implementation of the ctor *does not* call reserve(), it calls init() instead which deals with initial setup and calls allocateBuckets(), while reserve() on the other side calls grow(). I refactored the code that compute the number of buckets for a desired number of entries reserved, but this is the only thing shared.</div></div></div></blockquote><div><br class=""></div><div>Also:</div><div><br class=""></div><div>DenseMap Map(10); // allocate 64 buckets</div><div>DenseMap AnotherMap;</div><div>AnotherMap.reserve(10); // allocate 16 buckets</div><div><br class=""></div><div>The ctor and reserve don't have the same behavior, I don't mind doing more refactoring or cleanup (but I didn't mean to change this aspect of the behavior as part of this patch).</div><div><br class=""></div><div>-- </div><div>Mehdi</div><div><br class=""></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class=""><br class=""></div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">We can't really test all possible implementations today to ensure that future changes don't break the implementation - I agree, there's certainly some balance (testing generic edge cases that are likely to be hiccups, even if they aren't issues for today's algorithm). But I'd like there to be some more specific justification than a fairly unbounded "who knows how it might work tomorrow".<br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">The original test was already looping over an arbitrary amount of sizes, I don't know where to draw the line and I don't have any rational to drive any decision here, what do you suggest? </div><div class="">(I your concern is only runtime I can s/1024/128?)</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><br class="">(for the same reason we don't test, say, reserve after adding an element in each possible way an element can be added - we assume the way elements are added (but not the fact that they were added) is orthogonal to reserve)</div></div></div></blockquote><div class=""><br class=""></div><div class="">"Orthogonality" seems... orthogonal :) to what we're discussing here.</div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">><br class="">> ================<br class="">> Comment at: unittests/ADT/DenseMapTest.cpp:368<br class="">> @@ +367,3 @@<br class="">> +TEST(DenseMapCustomTest, InitialSizeTest) {<br class="">> + for (unsigned Size = 1; Size <= 1024; ++Size) {<br class="">> + DenseMap<unsigned, CountCopyAndMove> Map(Size);<br class="">> ----------------<br class="">> 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)<br class=""><br class=""></span>Runtime is 5ms for this test.<br class="">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)?<br class=""></blockquote><div class=""><br class=""></div><div class="">Well, what motivated you to choose 1024?<span class="Apple-converted-space"> </span></div></div></div></blockquote><div class=""><br class=""></div><div class="">I flipped a coin ;)</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">Why not 470 (is a power of two significant)?<span class="Apple-converted-space"> </span></div></div></div></blockquote><div class=""><br class=""></div><div class="">No good reason to choose a power of two.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">or 5 (is such a large number significant)?<span class="Apple-converted-space"> </span></div></div></div></blockquote><div class=""><br class=""></div><div class="">Size is significant (see below).</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">(sorry, I don't mean for that to sound facetious/confrontational, but I'd like to have some justification/discussion when adding test cases (in both directions, to be sure - to make sure we have enough testing, but also not too much)) <br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">That's fine! And I don't try to be difficult, I'm really not having any good justification or idea on how to "prune" smartly such a test.</div><div class="">What I can say is that the bug I fixed showed up only:</div><div class=""><br class=""></div><div class="">- showed up only for specific values (i.e. 4 * nEntries == 3 * nBuckets).</div><div class="">- the number of buckets had to be more than 64 (min initial alloc). </div><div class=""><br class=""></div><div class="">I didn't go with the minimum (48) because someone might figure that a minimum initialization of 512 for the map would be nice, I figure that 1024 was somehow reasonable, not that I was totally happy with it (and it was late last night...).</div><div class=""><br class=""></div><div class="">-- </div><div class="">Mehdi</div><div class=""><br class=""></div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><br class="">- Dave</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br class="">Thanks,<br class=""><br class="">Mehdi<br class=""><div class="HOEnZb"><div class="h5"><br class=""><br class=""><br class="">><br class="">> ================<br class="">> Comment at: unittests/ADT/DenseMapTest.cpp:377<br class="">> @@ +376,3 @@<br class="">> + EXPECT_EQ(Size * 2, CountCopyAndMove::Move);<br class="">> + EXPECT_EQ((unsigned)0, CountCopyAndMove::Copy);<br class="">> + }<br class="">> ----------------<br class="">> You could use '0u' for the unsigned 0 literal rather than a cast<br class="">><br class="">> ================<br class="">> Comment at: unittests/ADT/DenseMapTest.cpp:384<br class="">> @@ +383,3 @@<br class="">> +TEST(DenseMapCustomTest, ReserveTest) {<br class="">> + for (unsigned Size = 1; Size <= 1024; ++Size) {<br class="">> + DenseMap<unsigned, CountCopyAndMove> Map;<br class="">> ----------------<br class="">> Same question here<br class="">><br class="">><br class="">><span class="Apple-converted-space"> </span><a href="http://reviews.llvm.org/D18345" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D18345</a><br class="">><br class="">><br class="">><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></div></blockquote></div></div></blockquote></div><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@lists.llvm.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-commits@lists.llvm.org</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></body></html>