<div dir="ltr">SGTM.<br><div>I suggest we do it the way you suggest unless someone wants to opine in post-commit review.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Apr 9, 2016 at 2:11 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@chromium.org" target="_blank">jfb@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jfb added a comment.<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D18938#396391" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18938#396391</a>, @jfb wrote:<br>
<br>
> In <a href="http://reviews.llvm.org/D18938#396373" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18938#396373</a>, @jfb wrote:<br>
><br>
> > In <a href="http://reviews.llvm.org/D18938#396364" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18938#396364</a>, @dberlin wrote:<br>
> ><br>
</span><span class="">> > > I know these are all just hash values, so who cares, but in practice, you<br>
> > >  may just want to use an explicit cast to  std::underlying_type instead of<br>
> > >  uint64_t.<br>
> ><br>
> ><br>
</span><span class="">> > 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.<br>
> ><br>
> > Fixed that.<br>
><br>
><br>
> Well that was slightly broken, but not locally. I did a quick fix for the bots in <a href="http://reviews.llvm.org/rL265880" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL265880</a>, will ponder a yet-more-correct one.<br>
<br>
<br>
</span>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`:<br>
<br>
  /// Note that this function is intended to compute the same hash_code for<br>
  /// a particular value without regard to the pre-promotion type. This is in<br>
  /// contrast to hash_combine which may produce different hash_codes for<br>
  /// differing argument types even if they would implicit promote to a common<br>
  /// type without changing the value.<br>
<br>
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.<br>
<br>
WDYT?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D18938" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18938</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>