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

[Daniel Berlin via llvm-commits]

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


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

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160409/98440dc8/attachment.html>