<div dir="ltr">"the specified hash does not meet the Hash requirements" isn't a very actionable diagnostic. Is it possible to use some warning text that lets people know what they need to do to make the compiler happy, instead of just telling them that the compiler is unhappy?<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 3, 2017 at 5:35 PM, Eric Fiselier via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ericwf<br>
Date: Fri Mar  3 16:35:58 2017<br>
New Revision: 296919<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=296919&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=296919&view=rev</a><br>
Log:<br>
Fix hash requirements check in __hash_table.<br>
<br>
r296565 attempted to add better diagnostics when an unordered container<br>
is instantiated with a hash that doesn't meet the Hash requirements.<br>
<br>
However I mistakenly checked the wrong set of requirements. Specifically<br>
it checked if the hash met the requirements for specializations of<br>
std::hash. However these requirements are stricter than the generic<br>
Hash requirements.<br>
<br>
This patch fixes the assertions to only check the Hash requirements.<br>
<br>
Modified:<br>
    libcxx/trunk/include/__hash_<wbr>table<br>
    libcxx/trunk/include/utility<br>
    libcxx/trunk/test/libcxx/<wbr>containers/unord/unord.set/<wbr>missing_hash_specialization.<wbr>fail.cpp<br>
<br>
Modified: libcxx/trunk/include/__hash_<wbr>table<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__hash_table?rev=296919&r1=296918&r2=296919&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/libcxx/trunk/include/_<wbr>_hash_table?rev=296919&r1=<wbr>296918&r2=296919&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/include/__hash_<wbr>table (original)<br>
+++ libcxx/trunk/include/__hash_<wbr>table Fri Mar  3 16:35:58 2017<br>
@@ -871,16 +871,15 @@ public:<br>
 template <class _Key, class _Hash, class _Equal, class _Alloc><br>
 struct __diagnose_hash_table_helper {<br>
   static constexpr bool __trigger_diagnostics()<br>
-    _LIBCPP_DIAGNOSE_WARNING(__<wbr>has_enabled_hash<_Key, _Hash>::value<br>
+    _LIBCPP_DIAGNOSE_WARNING(__<wbr>check_hash_requirements<_Key, _Hash>::value<br>
                          && !__invokable<_Hash const&, _Key const&>::value,<br>
       "the specified hash functor does not provide a const call operator")<br>
     _LIBCPP_DIAGNOSE_WARNING(is_<wbr>copy_constructible<_Equal>::<wbr>value<br>
                           && !__invokable<_Equal const&, _Key const&, _Key const&>::value,<br>
       "the specified comparator type does not provide a const call operator")<br>
   {<br>
-    static_assert(__has_enabled_<wbr>hash<_Key, _Hash>::value,<br>
-      "the specified hash functor does not meet the requirements for an "<br>
-      "enabled hash");<br>
+    static_assert(__check_hash_<wbr>requirements<_Key, _Hash>::value,<br>
+      "the specified hash does not meet the Hash requirements");<br>
     static_assert(is_copy_<wbr>constructible<_Equal>::value,<br>
       "the specified comparator is required to be copy constructible");<br>
     return true;<br>
<br>
Modified: libcxx/trunk/include/utility<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/utility?rev=296919&r1=296918&r2=296919&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/libcxx/trunk/include/<wbr>utility?rev=296919&r1=296918&<wbr>r2=296919&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/include/utility (original)<br>
+++ libcxx/trunk/include/utility Fri Mar  3 16:35:58 2017<br>
@@ -1560,14 +1560,19 @@ struct _LIBCPP_TEMPLATE_VIS hash<nullptr<br>
 #endif<br>
<br>
 #ifndef _LIBCPP_CXX03_LANG<br>
-template <class _Key, class _Hash = std::hash<_Key> ><br>
-using __has_enabled_hash = integral_constant<bool,<br>
-    is_default_constructible<_<wbr>Hash>::value &&<br>
+template <class _Key, class _Hash><br>
+using __check_hash_requirements = integral_constant<bool,<br>
     is_copy_constructible<_Hash>::<wbr>value &&<br>
     is_move_constructible<_Hash>::<wbr>value &&<br>
     __invokable_r<size_t, _Hash, _Key const&>::value<br>
 >;<br>
<br>
+template <class _Key, class _Hash = std::hash<_Key> ><br>
+using __has_enabled_hash = integral_constant<bool,<br>
+    __check_hash_requirements<_<wbr>Key, _Hash>::value &&<br>
+    is_default_constructible<_<wbr>Hash>::value<br>
+>;<br>
+<br>
 #if _LIBCPP_STD_VER > 14<br>
 template <class _Type, class><br>
 using __enable_hash_helper_imp = _Type;<br>
<br>
Modified: libcxx/trunk/test/libcxx/<wbr>containers/unord/unord.set/<wbr>missing_hash_specialization.<wbr>fail.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/libcxx/containers/unord/unord.set/missing_hash_specialization.fail.cpp?rev=296919&r1=296918&r2=296919&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/libcxx/trunk/test/<wbr>libcxx/containers/unord/unord.<wbr>set/missing_hash_<wbr>specialization.fail.cpp?rev=<wbr>296919&r1=296918&r2=296919&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/test/libcxx/<wbr>containers/unord/unord.set/<wbr>missing_hash_specialization.<wbr>fail.cpp (original)<br>
+++ libcxx/trunk/test/libcxx/<wbr>containers/unord/unord.set/<wbr>missing_hash_specialization.<wbr>fail.cpp Fri Mar  3 16:35:58 2017<br>
@@ -23,14 +23,48 @@<br>
 #include <utility><br>
<br>
 using VT = std::pair<int, int>;<br>
-using Set = std::unordered_set<VT>;<br>
+<br>
+struct BadHashNoCopy {<br>
+  BadHashNoCopy() = default;<br>
+  BadHashNoCopy(BadHashNoCopy const&) = delete;<br>
+<br>
+  template <class T><br>
+  size_t operator()(T const&) const { return 0; }<br>
+};<br>
+<br>
+struct BadHashNoCall {<br>
+<br>
+};<br>
+<br>
+<br>
+struct GoodHashNoDefault {<br>
+  explicit GoodHashNoDefault(void*) {}<br>
+  template <class T><br>
+  size_t operator()(T const&) const { return 0; }<br>
+};<br>
<br>
 int main() {<br>
<br>
-  Set s; // expected-error@__hash_table:* {{the specified hash functor does not meet the requirements for an enabled hash}}<br>
+  {<br>
+    using Set = std::unordered_set<VT>;<br>
+    Set s; // expected-error@__hash_table:* {{the specified hash does not meet the Hash requirements}}<br>
+<br>
<br>
   // FIXME: It would be great to suppress the below diagnostic all together.<br>
   //        but for now it's sufficient that it appears last. However there is<br>
   //        currently no way to test the order diagnostics are issued.<br>
   // expected-error@memory:* {{call to implicitly-deleted default constructor of 'std::__1::hash<std::__1::<wbr>pair<int, int> >'}}<br>
+  }<br>
+  {<br>
+    using Set = std::unordered_set<int, BadHashNoCopy>;<br>
+    Set s; // expected-error@__hash_table:* {{the specified hash does not meet the Hash requirements}}<br>
+  }<br>
+  {<br>
+    using Set = std::unordered_set<int, BadHashNoCall>;<br>
+    Set s; // expected-error@__hash_table:* {{the specified hash does not meet the Hash requirements}}<br>
+  }<br>
+  {<br>
+    using Set = std::unordered_set<int, GoodHashNoDefault>;<br>
+    Set s(/*bucketcount*/42, GoodHashNoDefault(nullptr));<br>
+  }<br>
 }<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>