Page MenuHomePhabricator

[libcxx] Fix order checking in unordered_multiset tests.
ClosedPublic

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

Details

Summary

Some tests assume that iteration through an unordered multiset 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 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:57 AM
ldionne requested changes to this revision.Jan 28 2019, 12:30 PM
ldionne added inline comments.
test/std/containers/unord/unord.multiset/unord.multiset.cnstr/assign_copy.pass.cpp
67 ↗(On Diff #180867)

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.

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

Wouldn't it be enough (and simpler) to do something like this

No, it definitely wouldn't. The proposed simplification throws the baby out with the bathwater and lowers the testing value of the code. Both the original and the patched code effectively test these requirements of the standard. The simplified code lacks the check of the following requirement: "In containers that support equivalent keys, elements with equivalent keys are adjacent to each other in the iteration order of the container".

I find the proposed patch difficult to follow.

It is not the difficulty of the testing code that should matter, but its effectiveness and completeness called "test coverage". Тhe more you sweat in testing the less you bleed in application.

Is the patch rejected due to being "difficult to follow"?

If you have any further questions, please ask. If not, please accept and commit.

mclow.lists added inline comments.
test/std/containers/unord/unord.multiset/unord.multiset.cnstr/assign_copy.pass.cpp
67 ↗(On Diff #180867)

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.

mclow.lists added inline comments.Apr 19 2019, 7:38 AM
test/std/containers/unord/unord.multiset/unord.multiset.cnstr/assign_copy.pass.cpp
67 ↗(On Diff #180867)

Duh. Those calls would be:

CheckConsecutiveValues(c.find(1), c.end(), 1, 2);
CheckConsecutiveValues(c.find(2), c.end(), 2, 2);
CheckConsecutiveValues(c.find(3), c.end(), 3, 1);
CheckConsecutiveValues(c.find(4), c.end(), 4, 1);
amakc11 updated this revision to Diff 196072.Apr 22 2019, 7:40 AM

Suggested simplification implemented. It also implemented for the associated patch D56498.

amakc11 marked an inline comment as done.Apr 22 2019, 7:41 AM
amakc11 updated this revision to Diff 200799.May 22 2019, 11:27 AM

More simplifications accepted and committed in D56498 are implemented here: common function moved to a separate header file. Now this patch can also be accepted and committed.

amakc11 marked an inline comment as done.May 22 2019, 11:28 AM
ldionne accepted this revision.May 23 2019, 5:59 AM
This revision is now accepted and ready to land.May 23 2019, 5:59 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 6:08 AM