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

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists 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);
+}
----------------
Shouldn't you also assert here that `s.empty()`?


================
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");
----------------
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`.


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

https://reviews.llvm.org/D56498