[libcxx-commits] [PATCH] D56498: [libcxx] Fix order checking in some more unordered_multimap tests.

[Andrey Maksimov via Phabricator via libcxx-commits]

amakc11 added inline comments.


================
Comment at: test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp:48
+    }
+    assert(pos == end || pos->first != key);
+}
----------------
mclow.lists wrote:
> Shouldn't you also assert here that `s.empty()`?
This assert looks redundant. The loop iterates `vsize = values.size()` (assuming `vsize > 0`) times and calls `values.erase()` the same number of times. So, by the end of the loop, the set `values` would be empty. However, by default, I will add the assert if you still insist.


================
Comment at: test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp:85
         assert(c.size() == 6);
-        C::const_iterator i = c.cbegin();
-        assert(i->first == 1);
-        assert(i->second == "one");
-        ++i;
-        assert(i->first == 1);
-        assert(i->second == "four");
-        ++i;
-        assert(i->first == 2);
-        assert(i->second == "two");
-        ++i;
-        assert(i->first == 2);
-        assert(i->second == "four");
-        ++i;
-        assert(i->first == 3);
-        assert(i->second == "three");
-        ++i;
-        assert(i->first == 4);
-        assert(i->second == "four");
+        std::set<std::string> s;
+        s.insert("one");
----------------
mclow.lists wrote:
> I like this a lot better; thanks.
> 
> I don't think that `std::set` is the right container here to hold the strings. It assumes that is you don't have any duplicated strings.  That's true **for these tests** as they're written today, but could leave a ugly surprise for someone down the road. How about `std::multiset` instead?
> 
> Also, I'd rather you didn't pass `c` to `CheckConsecutiveKeys`; instead only passing two iterators. There's no need to actually involve the container when you're just checking that a sequence of iterators has a particular set of properties.
> 
> Besides, then testing the const/non-const case is as simple as passing `const_iterators` vs. `iterators`.
> How about `std::multiset` instead?
Agree, will do this.
> Also, I'd rather you didn't pass `c` to `CheckConsecutiveKeys`; instead only passing two iterators.
Here I tried to prevent your mistake "Duh. Those calls would be:" fixed in [[ https://reviews.llvm.org/D56500#1472704 | this your comment ]]. Passing `c` to `CheckConsecutiveKeys` avoids passing the same values multiple times. However, by default, I will revert to your initial suggestion with two iterators instead of one container.




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

https://reviews.llvm.org/D56498