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

[kboyarinov]

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

[kboyarinov]

Fixed incorrect upload with arc

[ldionne]

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

27

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

[kboyarinov]

In D113998#3134853, @ldionne wrote:

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?

[kboyarinov]

Fix CI, add return 0 where missed

[ldionne]

In D113998#3136978, @kboyarinov wrote:

In D113998#3134853, @ldionne wrote:

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.

[kboyarinov]

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

[kboyarinov]

In D113998#3140323, @ldionne wrote:

In D113998#3136978, @kboyarinov wrote:

In D113998#3134853, @ldionne wrote:

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]

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

[rarutyun]

In D113998#3148769, @ldionne wrote:

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.

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

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// key_compare key_comp() const;
-
-#include <map>
-#include <cassert>
-#include <string>
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    const map_type& cm = m;
-
-    assert(cm.key_comp()(p1.first->first, p2.first->first));
-    assert(!cm.value_comp()(p2.first->first, p1.first->first));
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// key_compare key_comp() const;
+
+#include <map>
+#include <cassert>
+#include <string>
+
+int main(int, char**) {
+    typedef std::map<int, std::string> map_type;
+
+    map_type m;
+    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
+    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
+
+    const map_type& cm = m;
+
+    assert(cm.key_comp()(p1.first->first, p2.first->first));
+    assert(!cm.key_comp()(p2.first->first, p1.first->first));
+}
 

libcxx/test/std/containers/associative/map/map.observers/value_comp.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// value_compare value_comp() const;
-
-#include <map>
-#include <cassert>
-#include <string>
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    const map_type& cm = m;
-
-    assert(cm.value_comp()(*p1.first, *p2.first));
-    assert(!cm.value_comp()(*p2.first, *p1.first));
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// value_compare value_comp() const;
+
+#include <map>
+#include <cassert>
+#include <string>
+
+int main(int, char**) {
+    typedef std::map<int, std::string> map_type;
+
+    map_type m;
+    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
+    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
+
+    const map_type& cm = m;
+
+    assert(cm.value_comp()(*p1.first, *p2.first));
+    assert(!cm.value_comp()(*p2.first, *p1.first));
+}
 

libcxx/test/std/containers/associative/map/map.value_compare/invoke.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// class value_compare
-
-// bool operator()( const value_type& lhs, const value_type& rhs ) const;
-
-#include <map>
-#include <cassert>
-#include <string>
-#include <utility>
-
-template <typename Map>
-struct CallCompMember : Map::value_compare {
-    CallCompMember(const typename Map::value_compare& vc) : Map::value_compare(vc) {}
-
-    typedef typename Map::value_type value_type;
-    bool operator()(const value_type& value1, const value_type& value2) const {
-        return this->comp(value1.first, value2.first);
-    }
-};
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    map_type::value_compare vc = m.value_comp();
-    CallCompMember<map_type> call_comp = m.value_comp();
-
-    assert(vc(*p1.first, *p2.first));
-    assert(call_comp(*p1.first, *p2.first));
-
-    assert(!vc(*p2.first, *p1.first));
-    assert(!call_comp(*p2.first, *p1.first));
-
-    return 0;
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// class value_compare
+
+// bool operator()( const value_type& lhs, const value_type& rhs ) const;
+
+#include <map>
+#include <cassert>
+#include <string>
+#include <utility>
+
+template <typename Map>
+struct CallCompMember : Map::value_compare {
+    CallCompMember(const typename Map::value_compare& vc) : Map::value_compare(vc) {}
+
+    typedef typename Map::value_type value_type;
+    bool operator()(const value_type& value1, const value_type& value2) const {
+        return this->comp(value1.first, value2.first);
+    }
+};
+
+int main(int, char**) {
+    typedef std::map<int, std::string> map_type;
+
+    map_type m;
+    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
+    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
+
+    map_type::value_compare vc = m.value_comp();
+    CallCompMember<map_type> call_comp = m.value_comp();
+
+    assert(vc(*p1.first, *p2.first));
+    assert(call_comp(*p1.first, *p2.first));
+
+    assert(!vc(*p2.first, *p1.first));
+    assert(!call_comp(*p2.first, *p1.first));
+
+    return 0;
+}
 

libcxx/test/std/containers/associative/map/map.value_compare/types.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// class value_compare
-
-// REQUIRES: c++98 || c++03 || c++11 || c++14 || c++17
-
-#include <map>
-#include <string>
-
-#include "test_macros.h"
-
-int main(int, char**) {
-    typedef map_type std::map<int, std::string>;
-    typedef value_compare map_type::value_compare;
-    typedef value_type map_type::value_type;
-
-    ASSERT_SAME_TYPE(value_compare::result_type, bool);
-    ASSERT_SAME_TYPE(value_compare::first_argument_type, value_type);
-    ASSERT_SAME_TYPE(value_compare::second_argument_type, value_type);
-
-    return 0;
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// class value_compare
+
+// REQUIRES: c++98 || c++03 || c++11 || c++14 || c++17
+
+#include <map>
+#include <string>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+    typedef std::map<int, std::string> map_type;
+    typedef map_type::value_compare value_compare;
+    typedef map_type::value_type value_type;
+
+    ASSERT_SAME_TYPE(value_compare::result_type, bool);
+    ASSERT_SAME_TYPE(value_compare::first_argument_type, value_type);
+    ASSERT_SAME_TYPE(value_compare::second_argument_type, value_type);
+
+    return 0;
+}
 

libcxx/test/std/containers/associative/multimap/multimap.observers/key_comp.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// key_compare key_comp() const;
-
-#include <map>
-#include <cassert>
-#include <string>
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    const map_type& cm = m;
-
-    assert(cm.key_comp()(p1.first->first, p2.first->first));
-    assert(!cm.value_comp()(p2.first->first, p1.first->first));
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// key_compare key_comp() const;
+
+#include <map>
+#include <cassert>
+#include <string>
+
+int main(int, char**) {
+    typedef std::multimap<int, std::string> map_type;
+
+    map_type m;
+    map_type::iterator i1 = m.insert(map_type::value_type(1, "abc"));
+    map_type::iterator i2 = m.insert(map_type::value_type(2, "abc"));
+
+    const map_type& cm = m;
+
+    assert(cm.key_comp()(i1->first, i2->first));
+    assert(!cm.key_comp()(i2->first, i1->first));
+}
 

libcxx/test/std/containers/associative/multimap/multimap.observers/value_comp.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// value_compare value_comp() const;
-
-#include <map>
-#include <cassert>
-#include <string>
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    const map_type& cm = m;
-
-    assert(cm.value_comp()(*p1.first, *p2.first));
-    assert(!cm.value_comp()(*p2.first, *p1.first));
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// value_compare value_comp() const;
+
+#include <map>
+#include <cassert>
+#include <string>
+
+int main(int, char**) {
+    typedef std::multimap<int, std::string> map_type;
+
+    map_type m;
+    map_type::iterator i1 = m.insert(map_type::value_type(1, "abc"));
+    map_type::iterator i2 = m.insert(map_type::value_type(2, "abc"));
+
+    const map_type& cm = m;
+
+    assert(cm.value_comp()(*i1, *i2));
+    assert(!cm.value_comp()(*i2, *i1));
+}
 

libcxx/test/std/containers/associative/multimap/multimap.value_compare/invoke.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// class value_compare
-
-// bool operator()( const value_type& lhs, const value_type& rhs ) const;
-
-#include <map>
-#include <cassert>
-#include <string>
-#include <utility>
-
-template <typename Map>
-struct CallCompMember : Map::value_compare {
-    CallCompMember(const typename Map::value_compare& vc) : Map::value_compare(vc) {}
-
-    typedef typename Map::value_type value_type;
-    bool operator()(const value_type& value1, const value_type& value2) const {
-        return this->comp(value1.first, value2.first);
-    }
-};
-
-int main(int, char**) {
-    typedef std::map<int, std::string> map_type;
-
-    map_type m;
-    std::pair<map_type::iterator, bool> p1 = m.insert(map_type::value_type(1, "abc"));
-    std::pair<map_type::iterator, bool> p2 = m.insert(map_type::value_type(2, "abc"));
-
-    map_type::value_compare vc = m.value_comp();
-    CallCompMember<map_type> call_comp = m.value_comp();
-
-    assert(vc(*p1.first, *p2.first));
-    assert(call_comp(*p1.first, *p2.first));
-
-    assert(!vc(*p2.first, *p1.first));
-    assert(!call_comp(*p2.first, *p1.first));
-
-    return 0;
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// class value_compare
+
+// bool operator()( const value_type& lhs, const value_type& rhs ) const;
+
+#include <map>
+#include <cassert>
+#include <string>
+#include <utility>
+
+template <typename MMap>
+struct CallCompMember : MMap::value_compare {
+    CallCompMember(const typename MMap::value_compare& vc) : MMap::value_compare(vc) {}
+
+    typedef typename MMap::value_type value_type;
+    bool operator()(const value_type& value1, const value_type& value2) const {
+        return this->comp(value1.first, value2.first);
+    }
+};
+
+int main(int, char**) {
+    typedef std::multimap<int, std::string> map_type;
+
+    map_type m;
+    map_type::iterator i1 = m.insert(map_type::value_type(1, "abc"));
+    map_type::iterator i2 = m.insert(map_type::value_type(2, "abc"));
+
+    map_type::value_compare vc = m.value_comp();
+    CallCompMember<map_type> call_comp = m.value_comp();
+
+    assert(vc(*i1, *i2));
+    assert(call_comp(*i1, *i2));
+
+    assert(!vc(*i2, *i1));
+    assert(!call_comp(*i2, *i1));
+
+    return 0;
+}
 

libcxx/test/std/containers/associative/multimap/multimap.value_compare/types.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <map>
-
-// class value_compare
-
-// REQUIRES: c++98 || c++03 || c++11 || c++14 || c++17
-
-#include <map>
-#include <string>
-
-#include "test_macros.h"
-
-int main(int, char**) {
-    typedef map_type std::map<int, std::string>;
-    typedef value_compare map_type::value_compare;
-    typedef value_type map_type::value_type;
-
-    ASSERT_SAME_TYPE(value_compare::result_type, bool);
-    ASSERT_SAME_TYPE(value_compare::first_argument_type, value_type);
-    ASSERT_SAME_TYPE(value_compare::second_argument_type, value_type);
-
-    return 0;
-}
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <map>
+
+// class value_compare
+
+// REQUIRES: c++98 || c++03 || c++11 || c++14 || c++17
+
+#include <map>
+#include <string>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+    typedef std::multimap<int, std::string> map_type;
+    typedef map_type::value_compare value_compare;
+    typedef map_type::value_type value_type;
+
+    ASSERT_SAME_TYPE(value_compare::result_type, bool);
+    ASSERT_SAME_TYPE(value_compare::first_argument_type, value_type);
+    ASSERT_SAME_TYPE(value_compare::second_argument_type, value_type);
+
+    return 0;
+}