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

[David Blaikie via llvm-commits]

On Tue, Mar 22, 2016 at 8:53 AM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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

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

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

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


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

Well, what motivated you to choose 1024? Why not 470 (is a power of two
significant)? or 5 (is such a large number significant)? (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))

- Dave


>
> 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
> >
> >
> >
>
> _______________________________________________
> 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/20160322/82cb2c24/attachment.html>