is_integral_or_enum ❥ enum class ⇒ hashable enum class

[jfb]

Summary

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.

This patch:

• Adds a check for is_enum when determining is_integral_or_enum.
• Explicitly converts the value parameter in hash_value to handle enum class hashing.

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.

[• dberlin]

This looks better than the hackery i came up with, so LGTM :)

One nit in case we want to care:

+ return ::llvm::hashing::detail::hash_integer_value((uint64_t)value);

So, you are guaranteed this value is either an enum, *or* convertible to
unsigned long long.

But what is the enum is not an unsigned type?

IE will this not still trigger on:

enum class e2: int {};

?

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.
or something. Again, my C++ knowledge mostly died after C++98, so not sure
what the "right" answer is here.

[jfb]

• Use underlying_type

[jfb]

In D18938#396364, @dberlin wrote:

This looks better than the hackery i came up with, so LGTM :)

One nit in case we want to care:

+ return ::llvm::hashing::detail::hash_integer_value((uint64_t)value);

So, you are guaranteed this value is either an enum, *or* convertible to
unsigned long long.

But what is the enum is not an unsigned type?

IE will this not still trigger on:

enum class e2: int {};

?

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.
or something. Again, my C++ knowledge mostly died after C++98, so not sure
what the "right" answer is here.

I was being a bit lazy because the signature is hash_code hash_integer_value(uint64_t value), but you're right using underlying_type is probably better if hash_integer_value's input type were to change.

Fixed that.

[jfb]

In D18938#396373, @jfb wrote:

In D18938#396364, @dberlin wrote:

This looks better than the hackery i came up with, so LGTM :)

One nit in case we want to care:

+ return ::llvm::hashing::detail::hash_integer_value((uint64_t)value);

So, you are guaranteed this value is either an enum, *or* convertible to
unsigned long long.

But what is the enum is not an unsigned type?

IE will this not still trigger on:

enum class e2: int {};

?

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.
or something. Again, my C++ knowledge mostly died after C++98, so not sure
what the "right" answer is here.

I was being a bit lazy because the signature is hash_code hash_integer_value(uint64_t value), but you're right using underlying_type is probably better if hash_integer_value's input type were to change.

Fixed that.

Well that was slightly broken, but not locally. I did a quick fix for the bots in http://reviews.llvm.org/rL265880, will ponder a yet-more-correct one.

[jfb]

In D18938#396391, @jfb wrote:

In D18938#396373, @jfb wrote:

In D18938#396364, @dberlin wrote:

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.

I was being a bit lazy because the signature is hash_code hash_integer_value(uint64_t value), but you're right using underlying_type is probably better if hash_integer_value's input type were to change.

Fixed that.

Well that was slightly broken, but not locally. I did a quick fix for the bots in http://reviews.llvm.org/rL265880, will ponder a yet-more-correct one.

OK I changed my mind on this and I think uint64_t is the right thing instead of template hackery to get the most restrictive type. My reasoning is based on the comment for hash_value:

/// Note that this function is intended to compute the same hash_code for
/// a particular value without regard to the pre-promotion type. This is in
/// contrast to hash_combine which may produce different hash_codes for
/// differing argument types even if they would implicit promote to a common
/// type without changing the value.

Getting underlying_type gets us the pre-promotion value and then implicitly converts it to uint64_t, whereas casting to uint64_t directly gets us the widest integral type supported here, so it seems to be the right type and follows the implicit conversion that was occurring previously.

WDYT?

[• dberlin]

SGTM.
I suggest we do it the way you suggest unless someone wants to opine in
post-commit review.

llvm/trunk/include/llvm/ADT/Hashing.h

 } // namespace detail
 } // namespace hashing
 
 // Declared and documented above, but defined here so that any of the hashing
 // infrastructure is available.
 template <typename T>
 typename std::enable_if<is_integral_or_enum<T>::value, hash_code>::type
 hash_value(T value) {
-  return ::llvm::hashing::detail::hash_integer_value(value);
+  return ::llvm::hashing::detail::hash_integer_value(
+      static_cast<typename std::underlying_type<T>::type>(value));
 }
 
 // Declared and documented above, but defined here so that any of the hashing
 // infrastructure is available.
 template <typename T> hash_code hash_value(const T *ptr) {
   return ::llvm::hashing::detail::hash_integer_value(
     reinterpret_cast<uintptr_t>(ptr));
 }

llvm/trunk/include/llvm/Support/type_traits.h

 
 // std::pair's are pod-like if their elements are.
 template<typename T, typename U>
 struct isPodLike<std::pair<T, U> > {
   static const bool value = isPodLike<T>::value && isPodLike<U>::value;
 };
 
 /// \brief Metafunction that determines whether the given type is either an
-/// integral type or an enumeration type.
+/// integral type or an enumeration type, including enum classes.
 ///
 /// Note that this accepts potentially more integral types than is_integral
-/// because it is based on merely being convertible implicitly to an integral
-/// type.
+/// because it is based on being implicitly convertible to an integral type.
+/// Also note that enum classes aren't implicitly convertible to integral types,
+/// the value may therefore need to be explicitly converted before being used.
 template <typename T> class is_integral_or_enum {
   typedef typename std::remove_reference<T>::type UnderlyingT;
 
 public:
   static const bool value =
       !std::is_class<UnderlyingT>::value && // Filter conversion operators.
       !std::is_pointer<UnderlyingT>::value &&
       !std::is_floating_point<UnderlyingT>::value &&
-      std::is_convertible<UnderlyingT, unsigned long long>::value;
+      (std::is_enum<UnderlyingT>::value ||
+       std::is_convertible<UnderlyingT, unsigned long long>::value);
 };
 
 /// \brief If T is a pointer, just return it. If it is not, return T&.
 template<typename T, typename Enable = void>
 struct add_lvalue_reference_if_not_pointer { typedef T &type; };
 
 template <typename T>
 struct add_lvalue_reference_if_not_pointer<