Page MenuHomePhabricator

[libcxx][NFC] Add tests for associative containers key_comp and value_comp
ClosedPublic

Authored by kboyarinov on Nov 16 2021, 7:04 AM.

Details

Summary

Add missing tests to improve associative containers code coverage:

  • Tests for key_comp() and value_comp() observers
  • Tests for std::map and std::multimap value_compare member class

Diff Detail

Event Timeline

kboyarinov requested review of this revision.Nov 16 2021, 7:04 AM
kboyarinov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 7:04 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
kboyarinov removed 1 blocking reviewer(s): Restricted Project.Nov 16 2021, 7:09 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptNov 16 2021, 7:09 AM

Fixed incorrect upload with arc

kboyarinov removed 1 blocking reviewer(s): Restricted Project.Nov 16 2021, 7:10 AM
ldionne requested changes to this revision.Nov 16 2021, 7:27 AM

I did find some tests in libcxx/test/std/containers/associative/multiset/multiset.cons/compare.pass.cpp (and similarly for other container types). Could we incorporate the existing tests into yours and delete the old ones? IMO your new tests are more complete, the only thing I would do is use test_less like in the original ones.

libcxx/test/std/containers/associative/map/map.observers/key_comp.pass.cpp
28

Please return 0; at the end of the function. In freestanding mode, main is not treated specially so we'd get a warning "not returning anything from a function that returns int".

This revision now requires changes to proceed.Nov 16 2021, 7:27 AM

I did find some tests in libcxx/test/std/containers/associative/multiset/multiset.cons/compare.pass.cpp (and similarly for other container types). Could we incorporate the existing tests into yours and delete the old ones? IMO your new tests are more complete, the only thing I would do is use test_less like in the original ones.

I am not sure that we can just remove the test compare.pass.cpp, because it is intended to test std::map(const key_compare&) constructor and uses std::map::key_comp to test the constructor effect. New test is for std::map::key_comp effects. I agree that these tests are connected, but I do not think they are interchangeable. What do you think?

Fix CI, add return 0 where missed

kboyarinov marked an inline comment as done.Nov 17 2021, 3:27 AM
ldionne accepted this revision.Nov 18 2021, 8:16 AM

I did find some tests in libcxx/test/std/containers/associative/multiset/multiset.cons/compare.pass.cpp (and similarly for other container types). Could we incorporate the existing tests into yours and delete the old ones? IMO your new tests are more complete, the only thing I would do is use test_less like in the original ones.

I am not sure that we can just remove the test compare.pass.cpp, because it is intended to test std::map(const key_compare&) constructor and uses std::map::key_comp to test the constructor effect. New test is for std::map::key_comp effects. I agree that these tests are connected, but I do not think they are interchangeable. What do you think?

Oh my, you are right, I looked too quickly and missed that we were testing the constructors. In those constructor tests, can you please remove the part of the comment that looks like:

// key_compare    key_comp() const;
// value_compare value_comp() const;

This confused me into thinking we were testing those methods there, instead of just the constructor (which is explained by the comment immediately above):

// explicit multiset(const value_compare& comp);
// value_compare and key_compare are the same type for set/multiset

LGTM with my suggested comment cleanup to the existing constructor tests.

This revision is now accepted and ready to land.Nov 18 2021, 8:16 AM
kboyarinov retitled this revision from [libcxx][NFC] Add tests for std::map and std::multimap key_comp and value_comp to [libcxx][NFC] Add tests for associative containers key_comp and value_comp.
kboyarinov edited the summary of this revision. (Show Details)

Clean-up for comments in compare.pass.cpp test for all associative containers

I did find some tests in libcxx/test/std/containers/associative/multiset/multiset.cons/compare.pass.cpp (and similarly for other container types). Could we incorporate the existing tests into yours and delete the old ones? IMO your new tests are more complete, the only thing I would do is use test_less like in the original ones.

I am not sure that we can just remove the test compare.pass.cpp, because it is intended to test std::map(const key_compare&) constructor and uses std::map::key_comp to test the constructor effect. New test is for std::map::key_comp effects. I agree that these tests are connected, but I do not think they are interchangeable. What do you think?

Oh my, you are right, I looked too quickly and missed that we were testing the constructors. In those constructor tests, can you please remove the part of the comment that looks like:

// key_compare    key_comp() const;
// value_compare value_comp() const;

This confused me into thinking we were testing those methods there, instead of just the constructor (which is explained by the comment immediately above):

// explicit multiset(const value_compare& comp);
// value_compare and key_compare are the same type for set/multiset

LGTM with my suggested comment cleanup to the existing constructor tests.

I have cleaned-up comments in compare.pass.cpp but I have figured out that this test was the only place where std::[multi]set::key_comp() and std::[multi]set::value_comp() observers were tested.
I have added the similar tests as for maps to have separate test for observers.

ldionne accepted this revision.Nov 23 2021, 6:37 AM

Thanks for the patch! Please provide "Author Name <email@domain>" if you want me to commit this on your behalf.

rarutyun accepted this revision.Nov 26 2021, 2:17 PM

Thanks for the patch! Please provide "Author Name <email@domain>" if you want me to commit this on your behalf.

Thanks Louis. No worries, I will commit that.