mrkite / minutor

Mapping for Minecraft
http://seancode.com/minutor
BSD 2-Clause "Simplified" License
278 stars 52 forks source link

don't use qHash() for permanent data, use CRC32 instead #326

Closed Kolcha closed 2 years ago

Kolcha commented 2 years ago

qHash() doesn't specify some algorithm, so it is not suitable for permanent data as so as returned value may be changed depending on Qt version (more correctly on used underlying algorithm)

CRC32 is very lightweight checksum algorithm, so it will not have significant impact to performance (especially for such small data)

CRC32 implementation is available in Zlib, which is already used in the project, so it costs nothing using CRC32 implementation from it

madmaxoft commented 2 years ago

Considering the amount of problems we had with "duplicate definitions" caused by changing the hash functions, this change has got me worried, will it cause another wave of those?

EtlamGit commented 2 years ago

CRC32 is also not a defined algorithm, you have to specify the polynom to get same results.

But what is more important: CRC is NOT a hash function. The purpose of a hash function is to generate results maximum apart for even tiny changes in input. This is a property to ensure avoiding collisions. CRC does not have this property, it is a very old algorithm to detect (single) transmission errors and to be extremely efficient to be implemented with discrete hardware gates. I would also assume that we have a higher likelihood to get collisions now.

My proposal would be to use a "real" hash function. Even MD5 or SHA1 are suited for our use case. They are designed for this purpose, they are very well tested and easy available. There is no strong need to use more modern ones like SHA256 as we do not do cryptography. Something that is used inside container (hash map) implementations is sufficient.