[libcxx-commits] [PATCH] D56500: [libcxx] Fix order checking in unordered_multiset tests.

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists added inline comments.


================
Comment at: test/std/containers/unord/unord.multiset/unord.multiset.cnstr/assign_copy.pass.cpp:67
         C::const_iterator i = c.cbegin();
-        assert(*i == 1);
-        ++i;
-        assert(*i == 1);
-        ++i;
-        assert(*i == 2);
-        ++i;
-        assert(*i == 2);
-        ++i;
-        assert(*i == 3);
-        ++i;
-        assert(*i == 4);
+        C::const_iterator ie = c.end();
+        std::set<int> s;
----------------
mclow.lists wrote:
> ldionne wrote:
> > Wouldn't it be enough (and simpler) to do something like this:
> > 
> > ```
> > std::map<int, int> counts;
> > for (C::const_iterator i = c.cbegin(), ie = c.cend(); i != ie; ++i) {
> >   counts[*i]++;
> > }
> > assert(counts[1] == 2);
> > assert(counts[2] == 2);
> > assert(counts[3] == 1);
> > assert(counts[4] == 1);
> > assert(counts.size() == 4);
> > ```
> > 
> > I find the proposed patch difficult to follow.
> > 
> I too find this patch difficult to follow. I don't really see what building and destroying the  `set s` adds.
> However, I agree that louis' suggested change does not test what we want.
> 
> Maybe something like this:
>     C::const_iterator i = c.find(1);
>     assert(i != c.end());
>     assert(*i == 1);
>     ++i;
>     assert(i != c.end());
>     assert(*i == 1);
>     ++i;
>     assert(i == c.end() || *i != 1);
> 
> and then repeat that code block for the values `2`, `3` and `4`, checking for the correct number of elements of each value.
> 
> You could even pull that checking out into a function like this:
>     template <typename Iter>
>     void CheckConsecutiveValues(Iter pos, Iter end, typename Iter::value_type value, size_t count);
> 
> and then just have here:
>     CheckConsecutiveValues(c.find(1), c.end(), 1, 2);
>     CheckConsecutiveValues(c.find(2), c.end(), 1, 2);
>     CheckConsecutiveValues(c.find(3), c.end(), 1, 1);
>     CheckConsecutiveValues(c.find(4), c.end(), 1, 1);
> 
> which I think would be very clear.
> 
Duh. Those calls would be:
    CheckConsecutiveValues(c.find(1), c.end(), 1, 2);
    CheckConsecutiveValues(c.find(2), c.end(), 2, 2);
    CheckConsecutiveValues(c.find(3), c.end(), 3, 1);
    CheckConsecutiveValues(c.find(4), c.end(), 4, 1);



Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56500/new/

https://reviews.llvm.org/D56500