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



Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56500