Issue #1080: Replace the original usage of string.GetHashCode() with …

[billchungiii]

Issue #1080: Replace the original usage of string.GetHashCode() with hash value calculation using the MurmurHash algorithm.

[drewnoakes]

Wonder if we could move this into a util class, like Strings, so it can be reused more easily in future.

[drewnoakes]

The implementation could be wrapped in unchecked { ... }.

[billchungiii]

Thank you for your suggestion, I have modified the code. 😊

[drewnoakes]

It can still throw if someone compiles in checked mode. The casts between int/uint need to be unchecked.

[billchungiii]

thanks for your reply, The result of hash % 55536 ranges from 0 to 55,535, which is a safe range since it does not exceed the positive integer limit. Therefore, (int)(hash % 55536) will not cause an overflow.

[drewnoakes]

I believe there are still two places that would fail checked arithmetic.

hash ^= (uint)(name[currentIndex + 2] << 16);

If the char has the high bit set, the result is a negative integer. Casting to uint will throw I think.

Similarly:

                uint k = (uint)(name[currentIndex] |
                                (name[currentIndex + 1] << 8) |
                                (name[currentIndex + 2] << 16) |
                                (name[currentIndex + 3] << 24));

Here's an example:

https://sharplab.io/#v2:EYLgHgbALANALiATgVwHYB8DGALAppga1wBMBYAKAG8KACOgAQEYA6JgTgAoPkBLVOAJQcARIB4NwCD7wgNoAGALo0APIpqMIAgQG4KAXyA=

The code seems to assume that char is int8 but they're actually int16.

[billchungiii]

Thanks for your advice, I modify the code.

[drewnoakes]

Murmur hash seems intended for use with bytes, but we have characters. How about using FNV hash instead, with something like:

public static int StableStringHash(string text)
{
    unchecked
    {
        const uint fnvOffset = 2166136261;
        const uint fnvPrime = 16777619;

        uint hash = fnvOffset;
        foreach (char c in text)
        {
            hash ^= (byte)(c & 0xFF);
            hash *= fnvPrime;
            hash ^= (byte)(c >> 8);
            hash *= fnvPrime;
        }

        return (int)hash;
    }
}

Again, I think this should be in a utility class, like Strings.

[drewnoakes]

This change means that applications using the newer code won't be able to connect with apps using the older code, even if both are on .NET Framework.

I think the best approach would therefore actually be to just use the same implementation of string hashing that exists in .NET Framework here.

[drewnoakes]

Please take a look at: #1122

[drewnoakes]

We want the compatibility offered by #1122, so I merged that approach. It also addresses the other feedback here (checked block and moved to utility class).

Thanks for getting things moving here.