Allowing Go runtime compilation to succeed under ARM #1990
Conversation
This fix allows compilation and execution under ARM architecture, since default type couldn't handle all possible values
| k1 *= c1_32 | ||
| k1 = (k1 << 15) | (k1 >> 17) // rotl32(k1, 15) | ||
| k1 *= c2_32 | ||
| var k1u uint |
There was a problem hiding this comment.
You can avoid defining the var if you use the := operator. (Here and elsewhere.)
| var h1u = uint(h1) ^ k1u | ||
| h1u = (h1u << 13) | (h1u >> 19) // rotl32(h1u, 13) | ||
| h1u = h1u*5 + 0xe6546b64 | ||
| return int(h1u) | ||
| } | ||
|
|
||
| func murmurFinish(h1 int, numberOfWords int) int { |
There was a problem hiding this comment.
Probably should fix the input type instead of relying on a cast (enforcing that there are no negative numbers passed to this function). Also might be good to document that numberOfWords is limited to be 4x less than the normal uint range.
There was a problem hiding this comment.
I started doing this but changing of the input arguments to uint percolates to many places, I don't have time to fix it at this moment, will push when it's ready
There was a problem hiding this comment.
i dont think it is needed. I did consider suggesting this option, then i checked alg specification, to my understanding h1 and k1 cannot be negative
Wikipedia code FTW
uint32_t murmur3_32(const uint8_t* key, size_t len, uint32_t seed) {
uint32_t h = seed;
if (len > 3) {
const uint32_t* key_x4 = (const uint32_t*) key;
size_t i = len >> 2;
do {
uint32_t k = *key_x4++;
k *= 0xcc9e2d51;
k = (k << 15) | (k >> 17);
k *= 0x1b873593;
h ^= k;
h = (h << 13) | (h >> 19);
h = (h * 5) + 0xe6546b64;
} while (--i);
key = (const uint8_t*) key_x4;
}
if (len & 3) {
size_t i = len & 3;
uint32_t k = 0;
key = &key[i - 1];
do {
k <<= 8;
k |= *key--;
} while (--i);
k *= 0xcc9e2d51;
k = (k << 15) | (k >> 17);
k *= 0x1b873593;
h ^= k;
}
h ^= len;
h ^= h >> 16;
h *= 0x85ebca6b;
h ^= h >> 13;
h *= 0xc2b2ae35;
h ^= h >> 16;
return h;
}
|
hi gang. Are we ready to merge this one? |
|
@milanaleksic @davesisson @neatnerd Are we ready to merge this one? |
|
Yes, I think this one is good to go. |
Discussion already present in #1858 - effectively the deduced type wasn't big enough to allow compilation and execution under my ARM server (Radxa Rock Pro). This change allowed compilation to succeed and I didn't find any issues so far
If tests are needed please hold the PR until I have time to add them.