[libc++] Improve diagnostics for non-const comparators and hashers in associative containers
Summary:
When providing a non-const-callable comparator in a map or set, the
warning diagnostic does not include the point of instantiation of
the container that triggered the warning, which makes it difficult
to track down the problem. This commit improves the diagnostic by
placing it directly in the body of the associative container.
The same change is applied to unordered associative containers, which
had a similar problem.
Finally, this commit cleans up the forward declarations of several
map and unordered_map helpers, which are not needed anymore.
<rdar://problem/41370747>
Reviewers: EricWF, mclow.lists
Subscribers: christof, dexonsmith, llvm-commits
Differential Revision: https://reviews.llvm.org/D48955
diff --git a/libcxx/docs/UsingLibcxx.rst b/libcxx/docs/UsingLibcxx.rst
index 41f4106..899656c 100644
--- a/libcxx/docs/UsingLibcxx.rst
+++ b/libcxx/docs/UsingLibcxx.rst
@@ -203,8 +203,10 @@
This macro disables the additional diagnostics generated by libc++ using the
`diagnose_if` attribute. These additional diagnostics include checks for:
- * Giving `set`, `map`, `multiset`, `multimap` a comparator which is not
- const callable.
+ * Giving `set`, `map`, `multiset`, `multimap` and their `unordered_`
+ counterparts a comparator which is not const callable.
+ * Giving an unordered associative container a hasher that is not const
+ callable.
**_LIBCPP_NO_VCRUNTIME**:
Microsoft's C and C++ headers are fairly entangled, and some of their C++
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 69ed8dd..6f5b183 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -35,15 +35,6 @@
template <class _Key, class _Tp>
struct __hash_value_type;
-template <class _Key, class _Cp, class _Hash,
- bool = is_empty<_Hash>::value && !__libcpp_is_final<_Hash>::value>
-class __unordered_map_hasher;
-
-template <class _Key, class _Cp, class _Pred,
- bool = is_empty<_Pred>::value && !__libcpp_is_final<_Pred>::value
- >
-class __unordered_map_equal;
-
#ifndef _LIBCPP_CXX03_LANG
template <class _Tp>
struct __is_hash_value_type_imp : false_type {};
@@ -418,7 +409,7 @@
_LIBCPP_DEBUG_MODE(__get_db()->__insert_i(this));
}
- _LIBCPP_INLINE_VISIBILITY
+ _LIBCPP_INLINE_VISIBILITY
__hash_const_iterator(const __non_const_iterator& __x) _NOEXCEPT
: __node_(__x.__node_)
{
@@ -871,35 +862,32 @@
};
#endif
+template <class _Key, class _Hash, class _Equal>
+struct __enforce_unordered_container_requirements {
#ifndef _LIBCPP_CXX03_LANG
-template <class _Key, class _Hash, class _Equal, class _Alloc>
-struct __diagnose_hash_table_helper {
- static constexpr bool __trigger_diagnostics()
- _LIBCPP_DIAGNOSE_WARNING(__check_hash_requirements<_Key, _Hash>::value
- && !__invokable<_Hash const&, _Key const&>::value,
- "the specified hash functor does not provide a const call operator")
- _LIBCPP_DIAGNOSE_WARNING(is_copy_constructible<_Equal>::value
- && !__invokable<_Equal const&, _Key const&, _Key const&>::value,
- "the specified comparator type does not provide a const call operator")
- {
static_assert(__check_hash_requirements<_Key, _Hash>::value,
- "the specified hash does not meet the Hash requirements");
+ "the specified hash does not meet the Hash requirements");
static_assert(is_copy_constructible<_Equal>::value,
- "the specified comparator is required to be copy constructible");
- return true;
- }
+ "the specified comparator is required to be copy constructible");
+#endif
+ typedef int type;
};
-template <class _Key, class _Value, class _Hash, class _Equal, class _Alloc>
-struct __diagnose_hash_table_helper<
- __hash_value_type<_Key, _Value>,
- __unordered_map_hasher<_Key, __hash_value_type<_Key, _Value>, _Hash>,
- __unordered_map_equal<_Key, __hash_value_type<_Key, _Value>, _Equal>,
- _Alloc>
-: __diagnose_hash_table_helper<_Key, _Hash, _Equal, _Alloc>
-{
-};
-#endif // _LIBCPP_CXX03_LANG
+template <class _Key, class _Hash, class _Equal>
+#ifndef _LIBCPP_CXX03_LANG
+ _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Equal const&, _Key const&, _Key const&>::value,
+ "the specified comparator type does not provide a const call operator")
+ _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Hash const&, _Key const&>::value,
+ "the specified hash functor does not provide a const call operator")
+#endif
+typename __enforce_unordered_container_requirements<_Key, _Hash, _Equal>::type
+__diagnose_unordered_container_requirements(int);
+
+// This dummy overload is used so that the compiler won't emit a spurious
+// "no matching function for call to __diagnose_unordered_xxx" diagnostic
+// when the overload above causes a hard error.
+template <class _Key, class _Hash, class _Equal>
+int __diagnose_unordered_container_requirements(void*);
template <class _Tp, class _Hash, class _Equal, class _Alloc>
class __hash_table
@@ -963,10 +951,6 @@
typedef allocator_traits<__pointer_allocator> __pointer_alloc_traits;
typedef typename __bucket_list_deleter::pointer __node_pointer_pointer;
-#ifndef _LIBCPP_CXX03_LANG
- static_assert(__diagnose_hash_table_helper<_Tp, _Hash, _Equal, _Alloc>::__trigger_diagnostics(), "");
-#endif
-
// --- Member data begin ---
__bucket_list __bucket_list_;
__compressed_pair<__first_node, __node_allocator> __p1_;
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index aa7370b..8148510 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -40,10 +40,6 @@
template <class _Key, class _Value>
struct __value_type;
-template <class _Key, class _CP, class _Compare,
- bool = is_empty<_Compare>::value && !__libcpp_is_final<_Compare>::value>
-class __map_value_compare;
-
template <class _Allocator> class __map_node_destructor;
template <class _TreeIterator> class _LIBCPP_TEMPLATE_VIS __map_iterator;
template <class _TreeIterator> class _LIBCPP_TEMPLATE_VIS __map_const_iterator;
@@ -966,24 +962,12 @@
};
+template<class _Tp, class _Compare>
#ifndef _LIBCPP_CXX03_LANG
-template <class _Tp, class _Compare, class _Allocator>
-struct __diagnose_tree_helper {
- static constexpr bool __trigger_diagnostics()
- _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
- "the specified comparator type does not provide a const call operator")
- { return true; }
-};
-
-template <class _Key, class _Value, class _KeyComp, class _Alloc>
-struct __diagnose_tree_helper<
- __value_type<_Key, _Value>,
- __map_value_compare<_Key, __value_type<_Key, _Value>, _KeyComp>,
- _Alloc
-> : __diagnose_tree_helper<_Key, _KeyComp, _Alloc>
-{
-};
-#endif // !_LIBCPP_CXX03_LANG
+ _LIBCPP_DIAGNOSE_WARNING(!std::__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
+ "the specified comparator type does not provide a const call operator")
+#endif
+int __diagnose_non_const_comparator();
template <class _Tp, class _Compare, class _Allocator>
class __tree
@@ -1855,10 +1839,6 @@
{
static_assert((is_copy_constructible<value_compare>::value),
"Comparator must be copy-constructible.");
-#ifndef _LIBCPP_CXX03_LANG
- static_assert((__diagnose_tree_helper<_Tp, _Compare, _Allocator>::
- __trigger_diagnostics()), "");
-#endif
destroy(__root());
}
diff --git a/libcxx/include/map b/libcxx/include/map
index d3c59f3..f6f2b68 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -486,7 +486,8 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-template <class _Key, class _CP, class _Compare, bool _IsSmall>
+template <class _Key, class _CP, class _Compare,
+ bool = is_empty<_Compare>::value && !__libcpp_is_final<_Compare>::value>
class __map_value_compare
: private _Compare
{
@@ -900,6 +901,7 @@
typedef value_type& reference;
typedef const value_type& const_reference;
+ static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
static_assert((is_same<typename allocator_type::value_type, value_type>::value),
"Allocator::value_type must be same type as value_type");
@@ -1626,6 +1628,7 @@
typedef value_type& reference;
typedef const value_type& const_reference;
+ static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
static_assert((is_same<typename allocator_type::value_type, value_type>::value),
"Allocator::value_type must be same type as value_type");
diff --git a/libcxx/include/set b/libcxx/include/set
index ccf785a..fb15ecf 100644
--- a/libcxx/include/set
+++ b/libcxx/include/set
@@ -445,6 +445,7 @@
typedef value_type& reference;
typedef const value_type& const_reference;
+ static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
static_assert((is_same<typename allocator_type::value_type, value_type>::value),
"Allocator::value_type must be same type as value_type");
@@ -925,6 +926,7 @@
typedef value_type& reference;
typedef const value_type& const_reference;
+ static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
static_assert((is_same<typename allocator_type::value_type, value_type>::value),
"Allocator::value_type must be same type as value_type");
diff --git a/libcxx/include/unordered_map b/libcxx/include/unordered_map
index 6f60749..5dd9236 100644
--- a/libcxx/include/unordered_map
+++ b/libcxx/include/unordered_map
@@ -414,7 +414,8 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-template <class _Key, class _Cp, class _Hash, bool _IsEmpty>
+template <class _Key, class _Cp, class _Hash,
+ bool = is_empty<_Hash>::value && !__libcpp_is_final<_Hash>::value>
class __unordered_map_hasher
: private _Hash
{
@@ -482,7 +483,8 @@
__x.swap(__y);
}
-template <class _Key, class _Cp, class _Pred, bool _IsEmpty>
+template <class _Key, class _Cp, class _Pred,
+ bool = is_empty<_Pred>::value && !__libcpp_is_final<_Pred>::value>
class __unordered_map_equal
: private _Pred
{
@@ -845,6 +847,7 @@
typedef const value_type& const_reference;
static_assert((is_same<value_type, typename allocator_type::value_type>::value),
"Invalid allocator::value_type");
+ static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
private:
typedef __hash_value_type<key_type, mapped_type> __value_type;
@@ -1667,6 +1670,7 @@
typedef const value_type& const_reference;
static_assert((is_same<value_type, typename allocator_type::value_type>::value),
"Invalid allocator::value_type");
+ static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
private:
typedef __hash_value_type<key_type, mapped_type> __value_type;
diff --git a/libcxx/include/unordered_set b/libcxx/include/unordered_set
index de23ca2..25b9220 100644
--- a/libcxx/include/unordered_set
+++ b/libcxx/include/unordered_set
@@ -384,6 +384,7 @@
typedef const value_type& const_reference;
static_assert((is_same<value_type, typename allocator_type::value_type>::value),
"Invalid allocator::value_type");
+ static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
private:
typedef __hash_table<value_type, hasher, key_equal, allocator_type> __table;
@@ -976,6 +977,7 @@
typedef const value_type& const_reference;
static_assert((is_same<value_type, typename allocator_type::value_type>::value),
"Invalid allocator::value_type");
+ static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
private:
typedef __hash_table<value_type, hasher, key_equal, allocator_type> __table;
diff --git a/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp b/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp
index ea0d9ac..ddd7960 100644
--- a/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp
+++ b/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp
@@ -27,7 +27,8 @@
static_assert(!std::__invokable<BadCompare const&, int const&, int const&>::value, "");
static_assert(std::__invokable<BadCompare&, int const&, int const&>::value, "");
- // expected-warning@__tree:* 4 {{the specified comparator type does not provide a const call operator}}
+ // expected-warning@set:* 2 {{the specified comparator type does not provide a const call operator}}
+ // expected-warning@map:* 2 {{the specified comparator type does not provide a const call operator}}
{
using C = std::set<int, BadCompare>;
C s;
diff --git a/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp b/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp
index 8adc675..f428ab9 100644
--- a/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp
+++ b/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp
@@ -11,7 +11,7 @@
// REQUIRES: diagnose-if-support, verify-support
// Test that libc++ generates a warning diagnostic when the container is
-// provided a non-const callable comparator.
+// provided a non-const callable comparator or a non-const hasher.
#include <unordered_set>
#include <unordered_map>
@@ -34,8 +34,10 @@
static_assert(!std::__invokable<BadEqual const&, int const&, int const&>::value, "");
static_assert(std::__invokable<BadEqual&, int const&, int const&>::value, "");
- // expected-warning@__hash_table:* 4 {{the specified comparator type does not provide a const call operator}}
- // expected-warning@__hash_table:* 4 {{the specified hash functor does not provide a const call operator}}
+ // expected-warning@unordered_set:* 2 {{the specified comparator type does not provide a const call operator}}
+ // expected-warning@unordered_map:* 2 {{the specified comparator type does not provide a const call operator}}
+ // expected-warning@unordered_set:* 2 {{the specified hash functor does not provide a const call operator}}
+ // expected-warning@unordered_map:* 2 {{the specified hash functor does not provide a const call operator}}
{
using C = std::unordered_set<int, BadHash, BadEqual>;
