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

[David Blaikie via llvm-commits]

On Tue, Mar 22, 2016 at 2:05 PM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> > On Mar 22, 2016, at 11:04 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > 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
>
>
> What about removing this default min size for reserve()?
>

Not sure I'm quite understanding the question. You mean in addition to
changes in the ctor for consistency? Sure


>
>
> --
> Mehdi
>
>
> _______________________________________________
> 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/20160323/a7c9f809/attachment.html>