Follow up changes to the type hash function

[chuan]

I missed a few comments from the previous PR (#943). This change addresses the remaining comments.

[senderista]

Just a couple naming/implementation suggestions.

[senderista]

std::rotl() is in C++20, but I don't think __rotl() is actually standardized? __builtin_rotateleft32() is defined in clang but apparently not in gcc, and I haven't found a cross-platform intrinsic in gcc that also works in clang. So I think we should just define our own inline rotl32() function in this file (we could factor it out if we need it anywhere else). You could use the final implementation from this post, which asserts to avoid UB: https://blog.regehr.org/archives/1063.

uint32_t rotl32(uint32_t x, uint32_t n)
{
  assert (n<32);
  return (x<<n) | (x>>(-n&31));
}

[senderista]

Actually, given that we don't have gcc compatibility as an official requirement at this point, I think you should just use __builtin_rotateleft32(). We can replace it with something like the code above if we decide to require gcc compatibility (if we're not already on C++20 by that point, in which case we'd just use std::rotl()).

[LaurentiuCristofor]

Or just keep the original macro.

[senderista]

I prefer the above version of rotl32() because it ensures that we aren't invoking UB (i.e. asserts that shift < word size). (The original macro also invokes UB for a 0-shift, which is likely to be common.) Anyway, the author of this version is an academic expert on undefined behavior. In the blog post I linked he evaluates several versions of this code, and concludes that the above version is the best for safety and performance. I'm willing to take his word for it.

That said, I think I still prefer just going with __builtin_rotateleft32(), perhaps with a comment linking to that blog post in case we later require gcc compatibility. But at this point I think either approach is acceptable and it's not worth the time to continue discussing this.

[LaurentiuCristofor]

LGTM but I agree with Tobin about not breaking compatibility with gcc here if possible, even if we might have broken it elsewhere.

[senderista]

Thanks for fixing the ROTL impl.