<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 3, 2017 at 4:01 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">"the specified hash does not meet the Hash requirements" isn't a very actionable diagnostic.</div></blockquote><div><br></div><div>True, For now I was only trying to out-diagnose template barf when I wrote it initially. So in that regard it's a bit more actionable that it was, at least I hope.</div><div><br></div><div>I'm planning to create some machinery to check and diagnose every hash requirement separately, but I was going to do that separately. For instance</div><div>some requirements could be diagnosed as warnings instead of hard errors, allowing users to keep their code compiling until they have time to fix it.</div><div><br></div><div>Of course patches are always welcome :-P</div><div><br></div><div>/Eric</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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="HOEnZb"><div class="h5"><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-pr<wbr>oject?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_ta<wbr>ble<br>
libcxx/trunk/include/utility<br>
libcxx/trunk/test/libcxx/conta<wbr>iners/unord/unord.set/missing_<wbr>hash_specialization.fail.cpp<br>
<br>
Modified: libcxx/trunk/include/__hash_ta<wbr>ble<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-pr<wbr>oject/libcxx/trunk/include/__<wbr>hash_table?rev=296919&r1=29691<wbr>8&r2=296919&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/include/__hash_ta<wbr>ble (original)<br>
+++ libcxx/trunk/include/__hash_ta<wbr>ble 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(__has<wbr>_enabled_hash<_Key, _Hash>::value<br>
+ _LIBCPP_DIAGNOSE_WARNING(__che<wbr>ck_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_c<wbr>opy_constructible<_Equal>::val<wbr>ue<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_ha<wbr>sh<_Key, _Hash>::value,<br>
- "the specified hash functor does not meet the requirements for an "<br>
- "enabled hash");<br>
+ static_assert(__check_hash_req<wbr>uirements<_Key, _Hash>::value,<br>
+ "the specified hash does not meet the Hash requirements");<br>
static_assert(is_copy_constru<wbr>ctible<_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-pr<wbr>oject/libcxx/trunk/include/uti<wbr>lity?rev=296919&r1=296918&r2=<wbr>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<_Hash<wbr>>::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<_Key<wbr>, _Hash>::value &&<br>
+ is_default_constructible<_Hash<wbr>>::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/conta<wbr>iners/unord/unord.set/missing_<wbr>hash_specialization.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-pr<wbr>oject/libcxx/trunk/test/libcxx<wbr>/containers/unord/unord.set/<wbr>missing_hash_specialization.<wbr>fail.cpp?rev=296919&r1=296918&<wbr>r2=296919&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/test/libcxx/conta<wbr>iners/unord/unord.set/missing_<wbr>hash_specialization.fail.cpp (original)<br>
+++ libcxx/trunk/test/libcxx/conta<wbr>iners/unord/unord.set/missing_<wbr>hash_specialization.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::pair<wbr><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" target="_blank">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>
</div></div></blockquote></div><br></div></div>