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

[JF Bastien via llvm-commits]

jfb added a comment.

In http://reviews.llvm.org/D18938#396391, @jfb wrote:

> In http://reviews.llvm.org/D18938#396373, @jfb wrote:
>
> > In http://reviews.llvm.org/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?


Repository:
  rL LLVM

http://reviews.llvm.org/D18938