Re: [PATCH] D18938: is_integral_or_enum ❥ enum class ⇒ hashable enum class

[Daniel Berlin via llvm-commits]

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.


On Sat, Apr 9, 2016 at 12:48 PM, JF Bastien <jfb at chromium.org> wrote:

> jfb created this revision.
> jfb added reviewers: dberlin, chandlerc.
> jfb added a subscriber: llvm-commits.
>
> 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.
>
> http://reviews.llvm.org/D18938
>
> Files:
>   include/llvm/ADT/Hashing.h
>   include/llvm/Support/type_traits.h
>
> Index: include/llvm/Support/type_traits.h
> ===================================================================
> --- include/llvm/Support/type_traits.h
> +++ include/llvm/Support/type_traits.h
> @@ -54,20 +54,22 @@
>  };
>
>  /// \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&.
> Index: include/llvm/ADT/Hashing.h
> ===================================================================
> --- include/llvm/ADT/Hashing.h
> +++ include/llvm/ADT/Hashing.h
> @@ -632,7 +632,7 @@
>  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((uint64_t)value);
>  }
>
>  // Declared and documented above, but defined here so that any of the
> hashing
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160409/1bce0050/attachment.html>