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#396373, @jfb wrote:

> In http://reviews.llvm.org/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.


Repository:
  rL LLVM

http://reviews.llvm.org/D18938