Page MenuHomePhabricator

[libcxx] Fix order checking in some more unordered_multimap tests.
ClosedPublic

Authored by amakc11 on Jan 9 2019, 9:20 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

amakc11 created this revision.Jan 9 2019, 9:20 AM
ldionne requested changes to this revision.Jan 28 2019, 12:38 PM
ldionne added inline comments.
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?

This revision now requires changes to proceed.Jan 28 2019, 12:38 PM

The answer is here. If you don't agree with it, please, suggest simpler patch with the same test coverage.

amakc11 updated this revision to Diff 196069.Apr 22 2019, 7:36 AM

Simplification suggested by Marshall Clow implemented.

mclow.lists added inline comments.
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.

amakc11 added inline comments.Apr 22 2019, 12:10 PM
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)

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

mclow.lists added inline comments.Apr 22 2019, 1:38 PM
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())
amakc11 updated this revision to Diff 196259.Apr 23 2019, 8:34 AM

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.

amakc11 marked 2 inline comments as done.Apr 23 2019, 8:35 AM

We are now very close to the end here. Let's finish this up.

If there are still questions/suggestions, please ask/post.

I am OK with this now, but @EricWF wants to weigh in.

Ping for @EricWF: project must go on.

Another personal ping for @EricWF. If you have any questions, please ask. If not, please accept and commit.

ldionne accepted this revision.May 17 2019, 7:56 AM

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.

This revision is now accepted and ready to land.May 17 2019, 7:56 AM

Would somebody commit this, please?

EricWF requested changes to this revision.May 17 2019, 10:34 PM

I have some concerns. Please don't commit this.

I'll follow up shortly.

This revision now requires changes to proceed.May 17 2019, 10:34 PM
EricWF accepted this revision.May 17 2019, 10:40 PM

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.

This revision is now accepted and ready to land.May 17 2019, 10:40 PM

I'll commit this is the morning.

amakc11 added inline comments.May 20 2019, 7:31 AM
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.

I'll commit this is the morning.

Is it morning yet?

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2019, 11:11 AM

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.

Thanks for suggestion. I'll consider it.