-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix -1 for BigInt factorial - Update gmp.jl #51497
Conversation
I noticed also this line can be eliminated: Line 20 in 66fe51f
i.e. 2 checks (branches) or even 1 (not 3), by adding 1 (or zero-based indexing; that will in effect happen, +1 optimized out), if thought to be worth it for one more entry in the lookup table. |
Add test for Lines 695 to 704 in 66fe51f
|
LGTM, other than the need for a |
@jmkuhn those tests are maybe in the wrong place. I added at test/gmp.jl, since I kind of want MPFR gone. Actually both. |
Is this issue still open? I also had the same approach, changing the factorial function in base/gmp.jl and the writing tests for it in test/gmp.jl. Its passed all checks when I tested it locally. Should I make a PR for this issue? |
Thanks for offering your help @KshitijThareja , but in this case this PR is already open for solving the issue, and we should rather move forward with it. |
Co-authored-by: Rafael Fourquet <[email protected]>
bump? test fails seem unrelated |
Fixes #51488