search

AlexSabourinDev / Mist_cpp-utility

A collection of useful c++ utilities for improved development speed and correctness.
MIT License
0 stars 0 forks source link

hashing algorithm unclear, possibly not working as intended #6

Closed xoorath closed 7 years ago

xoorath commented 7 years ago

include/utility/Hashing.h

I don't love the name of Hash64, as there are other 64 bit hashes (more on this in a moment). I'd suggest giving it an explicit name like Hash64_DJB2, and possibly using a macro or similar to have a simplified name for a good general purpose hash, even if it just points to Hash64_DJB2.

I would also change attribution from stack overflow to "Dan Bernstein" at http://www.cse.yorku.ca/~oz/hash.html

I haven't tried it, but the fact that your version returns a 64 bit integer and the source example returning a 32 bit integer seems very fishy to me for a hashing algorithm. Note that unsigned long long is not the same as unsigned long. Please do correct me if I'm wrong on this, but it looks like a mistake to me.

xoorath commented 7 years ago

And your constants, can't you have these be constexpr values inside your function? Remove them from the MIST namespace, unless you're using them elsewhere?

Edit: I see it's being used as a default parameter now. Wondering if an unnamed namespace will work to keep scope to just this file, or if that won't work in a header. I'm not sure....

namespace {
    // your constant here
}

Not sure.