This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add missing checks to tests for the move w/allocator constructors of associative containers.
ClosedPublic

Authored by amakc11 on Feb 7 2019, 9:35 AM.

Details

Summary

This patch is a continuation of rCXX352512. This revision appears to be incomplete. It makes possible for the source containers to remain not empty after move construction of another container. In this case, less deallocations will be registered by test_allocator with counter. So, checks on Counter_base::gConstructed also require modification provided by this patch.

Diff Detail

Event Timeline

amakc11 created this revision.Feb 7 2019, 9:35 AM
ldionne added inline comments.Feb 12 2019, 9:53 AM
test/std/containers/associative/map/map.cons/move_alloc.pass.cpp
181–182

Technically, I think the container could also have just 2 elements remaining, no? IOW, what you mean is really that num+6 <= Counter_base::gConstructed <= num+6+3?

amakc11 added inline comments.Feb 12 2019, 1:06 PM
test/std/containers/associative/map/map.cons/move_alloc.pass.cpp
181–182

This assumption looks very strange to me. The std::move() either deletes everything in the source container or preserves everything. Any intermediate solution when this function deletes any random(??) number of elements looks unreasonable. However, if you have any specific reasons for implementing (and testing) such intermediate solution, please, provide them. Please, also consult Marshall who is the author of these tests.

ldionne requested changes to this revision.Feb 12 2019, 1:32 PM
ldionne added inline comments.
test/std/containers/associative/map/map.cons/move_alloc.pass.cpp
181–182

Well.. the standard says this (http://eel.is/c++draft/lib.types.movedfrom):

Objects of types defined in the C++ standard library may be moved from ([class.copy.ctor]). Move operations may be explicitly specified or implicitly generated. Unless otherwise specified, such moved-from objects shall be placed in a valid but unspecified state.

I couldn't find something that said that std::map behaved differently, therefore as far as the Standard is concerned, a moved-from map must be left in a valid-but-unspecified state. @mclow.lists can double-check that I'm not mistaken here.

If I'm right, then leaving the moved-from map completely full, leaving it empty, or leaving any number of elements in it are all valid implementations. Therefore your patch is not really improving the test suite, it's just making the specific implementation you're testing pass. Instead, I think Counter_base::gConstructed <= num+6+m1.size() would be better.

This revision now requires changes to proceed.Feb 12 2019, 1:32 PM
amakc11 updated this revision to Diff 186664.Feb 13 2019, 7:35 AM

Agree. Patch modified as suggested.

amakc11 marked an inline comment as done.Feb 13 2019, 7:35 AM
ldionne accepted this revision.Feb 13 2019, 8:34 AM
This revision is now accepted and ready to land.Feb 13 2019, 8:34 AM
ldionne closed this revision.Feb 13 2019, 8:43 AM

r353955