<div dir="ltr">This looks better than the hackery i came up with, so LGTM :)<div><br></div><div>One nit in case we want to care:</div><div><br><br><div><span style="font-size:12.8px">+ return ::llvm::hashing::detail::hash_</span><span style="font-size:12.8px">integer_value((uint64_t)value)</span><span style="font-size:12.8px">;</span><br style="font-size:12.8px"></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">So, you are guaranteed this value is either an enum, *or* convertible to unsigned long long.</span></div><div><span style="font-size:12.8px"><br>But what is the enum is not an unsigned type?<br><br></span></div></div><div><span style="font-size:12.8px">IE will this not still trigger on:<br></span><pre class="" style="padding:0px;border:0px none white;color:rgb(0,0,0);line-height:1.2em;border-radius:5px;margin-top:0px;margin-bottom:0px;width:55em;overflow:auto;font-stretch:normal;font-size:12.8px;vertical-align:top;background:none rgb(249,249,249)"><span class="" style="color:rgb(0,0,255)">enum</span> <span class="" style="color:rgb(0,0,221)">class</span> e2<span class="" style="color:rgb(0,128,128)">:</span> <span class="" style="color:rgb(0,0,255)">int</span> <span class="" style="color:rgb(0,128,0)">{</span><span class="" style="color:rgb(0,128,0)">}</span><span class="" style="color:rgb(0,128,128)">;</span>
</pre></div><div><span class="" style="color:rgb(0,128,128)"><br></span></div><div>?</div><div><br></div><div>I know these are all just hash values, so who cares, but in practice, you may just want to use an explicit cast to std::underlying_type instead of uint64_t.</div><div>or something. Again, my C++ knowledge mostly died after C++98, so not sure what the "right" answer is here.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Apr 9, 2016 at 12:48 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@chromium.org" target="_blank">jfb@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jfb created this revision.<br>
jfb added reviewers: dberlin, chandlerc.<br>
jfb added a subscriber: llvm-commits.<br>
<br>
As discussed in D18775 making AtomicOrdering an enum class makes it non-hashable, which shouldn't be the case. Hashing.h defines hash_value for all is_integral_or_enum, but type_traits.h's definition of is_integral_or_enum only checks for *inplicit* conversion to integral types which leaves enum classes out and is very confusing because is_enum is true for enum classes.<br>
<br>
This patch:<br>
- Adds a check for is_enum when determining is_integral_or_enum.<br>
- Explicitly converts the value parameter in hash_value to handle enum class hashing.<br>
<br>
Note that the warning at the top of Hashing.h still applies: each execution of the program has a high probability of producing a different hash_code for a given input. Thus their values are not stable to save or persist, and should only be used during the execution for the construction of hashing datastructures.<br>
<br>
<a href="http://reviews.llvm.org/D18938" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18938</a><br>
<br>
Files:<br>
include/llvm/ADT/Hashing.h<br>
include/llvm/Support/type_traits.h<br>
<br>
Index: include/llvm/Support/type_traits.h<br>
===================================================================<br>
--- include/llvm/Support/type_traits.h<br>
+++ include/llvm/Support/type_traits.h<br>
@@ -54,20 +54,22 @@<br>
};<br>
<br>
/// \brief Metafunction that determines whether the given type is either an<br>
-/// integral type or an enumeration type.<br>
+/// integral type or an enumeration type, including enum classes.<br>
///<br>
/// Note that this accepts potentially more integral types than is_integral<br>
-/// because it is based on merely being convertible implicitly to an integral<br>
-/// type.<br>
+/// because it is based on being implicitly convertible to an integral type.<br>
+/// Also note that enum classes aren't implicitly convertible to integral types,<br>
+/// the value may therefore need to be explicitly converted before being used.<br>
template <typename T> class is_integral_or_enum {<br>
typedef typename std::remove_reference<T>::type UnderlyingT;<br>
<br>
public:<br>
static const bool value =<br>
!std::is_class<UnderlyingT>::value && // Filter conversion operators.<br>
!std::is_pointer<UnderlyingT>::value &&<br>
!std::is_floating_point<UnderlyingT>::value &&<br>
- std::is_convertible<UnderlyingT, unsigned long long>::value;<br>
+ (std::is_enum<UnderlyingT>::value ||<br>
+ std::is_convertible<UnderlyingT, unsigned long long>::value);<br>
};<br>
<br>
/// \brief If T is a pointer, just return it. If it is not, return T&.<br>
Index: include/llvm/ADT/Hashing.h<br>
===================================================================<br>
--- include/llvm/ADT/Hashing.h<br>
+++ include/llvm/ADT/Hashing.h<br>
@@ -632,7 +632,7 @@<br>
template <typename T><br>
typename std::enable_if<is_integral_or_enum<T>::value, hash_code>::type<br>
hash_value(T value) {<br>
- return ::llvm::hashing::detail::hash_integer_value(value);<br>
+ return ::llvm::hashing::detail::hash_integer_value((uint64_t)value);<br>
}<br>
<br>
// Declared and documented above, but defined here so that any of the hashing<br>
<br>
<br>
</blockquote></div><br></div>