davedelong / DDMathParser

String → Number
MIT License
856 stars 153 forks source link

Any factorial above 33! returns 0, and extremely large factorials freeze my app. #95

Closed ghost closed 9 years ago

ghost commented 9 years ago

I'm using DDMathParser in my calculator app, and every time the user enters any factorial above 33!, numberByEvaluatingString() returns 0. Also, if the user enters an extremely large factorial, my app freezes and the CPU usage shoots straight to 100%.

liaujianjie commented 9 years ago

I tried troubleshooting this and found that the error occurs from factorial(21) to larger integer factorials. In _DDFunctionEvaluator.m, I found something really odd while performing a calculation for factorial(21). In - (DDExpression *)factorial:(NSArray *)arguments variables:(NSDictionary *)variables error:(NSError **)error,

This:

NSUInteger total = 1;
NSUInteger integer = [firstValue unsignedIntegerValue];
while (integer > 2) { // I changed this because when integer = 2 it will take total multiplied by 1
    total *= integer--;
    NSLog(@"%ld %ld",(long)integer,(long)total);
}

Logs this:

...
5 425757851430912000
4 2128789257154560000
3 8515157028618240000
2 7098727012145168384 <-- (supposed to be 2432902008176640000)

Using doubles for the variables total and integer logs this:

...
5 425757851430912000
4 2128789257154560000
3 8515157028618240000
2 9223372036854775807 <-- (supposed to be 2432902008176640000)

Notice how both double and NSUInteger give the same inaccuracy issue for factorial(21). I'm not sure why this is happening.

However if you want a temporary fix, you can remove the chunk where it checks if the number is an integer and just use the tgamma() function.

davedelong commented 9 years ago

Yes, anything about 20! will result in an incorrect value, because we're limited to 64 bits of precision. That means the largest integer factorial that can be represented is 20!, which is 2,432,902,008,176,640,000. 21! is 51,090,942,171,709,440,000, which is larger than the largest representable 64-bit integer of 18,446,744,073,709,551,616. I'll have it default back to tgamma if it's trying to compute an integer factorial larger than 20

ghost commented 9 years ago

Thank you both for the response! This was very helpful for me!