Some tests assume that iteration through an unordered multimap elements will return them in the same order as at the container creation. This assumption is not true since the container is unordered, so that no specific order of elements is ever guaranteed for such container. This patch is a continuation of D54838 and introduces checks verifying that any iteration will return elements exactly from a set of valid values and without repetition, but in no particular order.
Details
- Reviewers
EricWF ldionne - Commits
- rZORGa2a4daa0e510: [libcxx][test] Fix order checking in some more unordered_multimap tests
rGa2a4daa0e510: [libcxx][test] Fix order checking in some more unordered_multimap tests
rGc44cd1e4ed9c: [libcxx][test] Fix order checking in some more unordered_multimap tests
rCXX361414: [libcxx][test] Fix order checking in some more unordered_multimap tests
rL361414: [libcxx][test] Fix order checking in some more unordered_multimap tests
Diff Detail
- Repository
- rL LLVM
Event Timeline
test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp | ||
---|---|---|
68 ↗ | (On Diff #180860) | Is there any way you could simplify this by e.g. inserting into a std::map or std::multimap? |
The answer is here. If you don't agree with it, please, suggest simpler patch with the same test coverage.
test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp | ||
---|---|---|
48 ↗ | (On Diff #196069) | Shouldn't you also assert here that s.empty()? |
85 ↗ | (On Diff #196069) | 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. |
test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp | ||
---|---|---|
48 ↗ | (On Diff #196069) | 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. |
85 ↗ | (On Diff #196069) |
Agree, will do this.
Here I tried to prevent your mistake "Duh. Those calls would be:" fixed in 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. |
test/std/containers/unord/unord.multimap/unord.multimap.cnstr/assign_copy.pass.cpp | ||
---|---|---|
48 ↗ | (On Diff #196069) | Oh, I see. I was passing in a count of values; you're passing in a set of values, and "getting the count" from the set. My misunderstanding. In that case, I would change the loop to be: while (!values.empty()) |
Suggestions implemented. Common functions moved to a separate header file. This header will also be used to simplify code in patch D56500 after this one is accepted and committed.
Another personal ping for @EricWF. If you have any questions, please ask. If not, please accept and commit.
FWIW, I'm now happy with this. My original push back was due to the vast increase in the test complexity, but now it's much better.
Actually, looking again. This is good.
Thanks for the change.
test/std/containers/check_consecutive.h | ||
---|---|---|
1 ↗ | (On Diff #196259) | Please put this in test/support, which is on the include paths for the tests. That way you don't have to include it via relative path. |
test/std/containers/check_consecutive.h | ||
---|---|---|
1 ↗ | (On Diff #196259) | First, my primary goal when making patches to tests is to be fully consistent with the original style of the original author of each test. In the tests in question, there is a lot of code like this: #include "../../../test_compare.h" #include "../../../test_hash.h" My patch is consistent with it, isn't it? Second, moving those headers (all of them, to be consistent) to another directory is a target for refactoring which might be a goal of another patch. Let's move step by step and not mix multiple tasks in one single patch. |
Yet another personal ping for @EricWF. Let's finish up with this finally and proceed with the project.
I'm going to commit this for you, however you should consider getting commit access (https://llvm.org/docs/DeveloperPolicy.html) since you've done a couple of patches. As you can read in the policy, this doesn't mean you can commit just whatever you want, however you could commit stuff yourself after getting the review approved.