[LLVMdev] ADT/Hashing.h on 32-bit platforms

[Stephan Tolksdorf]

On 14-02-03 23:10, Chandler Carruth wrote:
> A couple of minor comments about the details of the patch:
>
> -  hash_state state = state.create(s_begin, seed);
> +  hash_state state = hash_state::create(s_begin, seed);
>
> Why this change? The existing code is correct already I think?

Is this a common idiom? On first sight this looked like a "self 
initialization" bug to me, so I thought it might make the code more 
readable to clarify that create is a static member function of 
hash_state. But I probably should have put such a code style "cleanup" 
into a separate patch.

> -  return ::llvm::hashing::detail::hash_integer_value(value);
> +  // the cast is necessary for strong enums and avoids compiler warnings
>
> Generally LLVM comments should be formed as prose (starting with a
> capitalized word and ending with appropriate punctuation). I don't think
> you need a comment here though, or a separate variable. I would just
> explicitly cast the value in the argument.
>
> +  const uint64_t n = static_cast<uint64_t>(value);
> +  return ::llvm::hashing::detail::hash_integer_value(n);

Thanks. Since there previously was no explicit cast I thought it might 
be useful to explain why I inserted one. I put in the separate variable, 
so that I'd have a good place to put my comment on without exceeding the 
column limit :-)

Let me know if you'd like me to update my patch. I'm happy to make any 
changes, but since I can't commit it myself, I'm not sure what would be 
easier for you.