appsquickly / typhoon

Powerful dependency injection for Objective-C ✨✨ (https://PILGRIM.PH is the pure Swift successor to Typhoon!!)✨✨
https://pilgrim.ph
Apache License 2.0
2.7k stars 270 forks source link

Invalid shift exponent in `calculateHash` #581

Closed daria-kopaliani closed 5 years ago

daria-kopaliani commented 6 years ago
- (NSUInteger)calculateHash
{
    NSUInteger hash = (NSUInteger) sel_getName(_selector);

    for (id <TyphoonParameterInjection> parameter in _injectedParameters) {
        hash = (NSUInteger) ((5u << hash) - hash + [[parameter description] hash]);
    }

    return hash;
}

method results in

shift exponent 4306227661 is too large for 32-bit type unsigned int

I'll be happy to make a pull request.

alexgarbarev commented 6 years ago

Wow! Nice catch. Of course, any PR is very welcome!

alexgarbarev commented 6 years ago

Hey @daria-kopaliani, I reviewed the code again, I can't understand why this shift is invalid?

This is "31 multiply" hash function well known in Java world, https://docs.oracle.com/javase/6/docs/api/java/lang/String.html#hashCode()

So the hash function is hash = 31 * hash + value or hash = hash << 5 - hash + value See more details in this answer: https://stackoverflow.com/a/299748/1817391

I suggest to do 31 multiplication if shift is not working anymore.


Meanwhile XOR is not good for hash, because A ^ B == B ^ A

alexgarbarev commented 5 years ago

Hello @daria-kopaliani, sorry for long delay. I'm going to fix this issue. Can you remember how this issue happened? Was it 32bit device? I'm unable to replicate it.

alexgarbarev commented 5 years ago

Well.. I found the reason of that problem. There is a typo 5 << hash instead of hash << 5 🤦, so instead of shifting hash by 5 bits to right, I shifted 5 by hash bits to right. It looks like your compile flags were set to stop when shifting happens by more than 32 bits at once (as I know it's unexpected behaviour)..