From 5fe0a6a0bc22bd48d4d90f4b45fc26098a742347 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Thu, 6 Dec 2018 21:46:17 +0000 Subject: [PATCH] [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. Reviewers: EricWF, mclow.lists Subscribers: christof, dexonsmith, llvm-commits Differential Revision: https://reviews.llvm.org/D48955 git-svn-id: https://llvm.org/svn/llvm-project/libcxx/trunk@348529 91177308-0d34-0410-b5e6-96231b3b80d8 --- docs/UsingLibcxx.rst | 6 +- include/__hash_table | 60 +++++++------------ include/__tree | 30 ++-------- include/map | 5 +- include/set | 2 + include/unordered_map | 8 ++- include/unordered_set | 2 + .../associative/non_const_comparator.fail.cpp | 3 +- .../unord/non_const_comparator.fail.cpp | 8 ++- 9 files changed, 52 insertions(+), 72 deletions(-) diff --git a/docs/UsingLibcxx.rst b/docs/UsingLibcxx.rst index 41f410684..899656cca 100644 --- a/docs/UsingLibcxx.rst +++ b/docs/UsingLibcxx.rst @@ -203,8 +203,10 @@ thread safety annotations. 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/include/__hash_table b/include/__hash_table index 69ed8dd8b..6f5b18310 100644 --- a/include/__hash_table +++ b/include/__hash_table @@ -35,15 +35,6 @@ _LIBCPP_BEGIN_NAMESPACE_STD template struct __hash_value_type; -template ::value && !__libcpp_is_final<_Hash>::value> -class __unordered_map_hasher; - -template ::value && !__libcpp_is_final<_Pred>::value - > -class __unordered_map_equal; - #ifndef _LIBCPP_CXX03_LANG template struct __is_hash_value_type_imp : false_type {}; @@ -418,7 +409,7 @@ public: _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 @@ struct __generic_container_node_destructor<__hash_node<_Tp, _VoidPtr>, _Alloc> }; #endif +template +struct __enforce_unordered_container_requirements { #ifndef _LIBCPP_CXX03_LANG -template -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 -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 +#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 +int __diagnose_unordered_container_requirements(void*); template class __hash_table @@ -963,10 +951,6 @@ private: 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/include/__tree b/include/__tree index aa7370bfb..814851085 100644 --- a/include/__tree +++ b/include/__tree @@ -40,10 +40,6 @@ template class __tree_node; template struct __value_type; -template ::value && !__libcpp_is_final<_Compare>::value> -class __map_value_compare; - template class __map_node_destructor; template class _LIBCPP_TEMPLATE_VIS __map_iterator; template class _LIBCPP_TEMPLATE_VIS __map_const_iterator; @@ -966,24 +962,12 @@ private: }; +template #ifndef _LIBCPP_CXX03_LANG -template -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 -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 __tree @@ -1855,10 +1839,6 @@ __tree<_Tp, _Compare, _Allocator>::~__tree() { static_assert((is_copy_constructible::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/include/map b/include/map index d3c59f3c5..f6f2b6835 100644 --- a/include/map +++ b/include/map @@ -486,7 +486,8 @@ swap(multimap& x, _LIBCPP_BEGIN_NAMESPACE_STD -template +template ::value && !__libcpp_is_final<_Compare>::value> class __map_value_compare : private _Compare { @@ -900,6 +901,7 @@ public: typedef value_type& reference; typedef const value_type& const_reference; + static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), ""); static_assert((is_same::value), "Allocator::value_type must be same type as value_type"); @@ -1626,6 +1628,7 @@ public: typedef value_type& reference; typedef const value_type& const_reference; + static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), ""); static_assert((is_same::value), "Allocator::value_type must be same type as value_type"); diff --git a/include/set b/include/set index ccf785ab1..fb15ecfa0 100644 --- a/include/set +++ b/include/set @@ -445,6 +445,7 @@ public: typedef value_type& reference; typedef const value_type& const_reference; + static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), ""); static_assert((is_same::value), "Allocator::value_type must be same type as value_type"); @@ -925,6 +926,7 @@ public: typedef value_type& reference; typedef const value_type& const_reference; + static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), ""); static_assert((is_same::value), "Allocator::value_type must be same type as value_type"); diff --git a/include/unordered_map b/include/unordered_map index 6f60749c9..5dd923607 100644 --- a/include/unordered_map +++ b/include/unordered_map @@ -414,7 +414,8 @@ template _LIBCPP_BEGIN_NAMESPACE_STD -template +template ::value && !__libcpp_is_final<_Hash>::value> class __unordered_map_hasher : private _Hash { @@ -482,7 +483,8 @@ swap(__unordered_map_hasher<_Key, _Cp, _Hash, __b>& __x, __x.swap(__y); } -template +template ::value && !__libcpp_is_final<_Pred>::value> class __unordered_map_equal : private _Pred { @@ -845,6 +847,7 @@ public: typedef const value_type& const_reference; static_assert((is_same::value), "Invalid allocator::value_type"); + static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), ""); private: typedef __hash_value_type __value_type; @@ -1667,6 +1670,7 @@ public: typedef const value_type& const_reference; static_assert((is_same::value), "Invalid allocator::value_type"); + static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), ""); private: typedef __hash_value_type __value_type; diff --git a/include/unordered_set b/include/unordered_set index de23ca2a3..25b92203f 100644 --- a/include/unordered_set +++ b/include/unordered_set @@ -384,6 +384,7 @@ public: typedef const value_type& const_reference; static_assert((is_same::value), "Invalid allocator::value_type"); + static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), ""); private: typedef __hash_table __table; @@ -976,6 +977,7 @@ public: typedef const value_type& const_reference; static_assert((is_same::value), "Invalid allocator::value_type"); + static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), ""); private: typedef __hash_table __table; diff --git a/test/libcxx/containers/associative/non_const_comparator.fail.cpp b/test/libcxx/containers/associative/non_const_comparator.fail.cpp index ea0d9ac09..ddd796089 100644 --- a/test/libcxx/containers/associative/non_const_comparator.fail.cpp +++ b/test/libcxx/containers/associative/non_const_comparator.fail.cpp @@ -27,7 +27,8 @@ int main() { static_assert(!std::__invokable::value, ""); static_assert(std::__invokable::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; C s; diff --git a/test/libcxx/containers/unord/non_const_comparator.fail.cpp b/test/libcxx/containers/unord/non_const_comparator.fail.cpp index 8adc67589..f428ab9ac 100644 --- a/test/libcxx/containers/unord/non_const_comparator.fail.cpp +++ b/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 #include @@ -34,8 +34,10 @@ int main() { static_assert(!std::__invokable::value, ""); static_assert(std::__invokable::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;