Skip to content
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

Generate instead of hard coding factorial tables #22096

Merged
merged 3 commits into from
May 30, 2017

Conversation

TotalVerb
Copy link
Contributor

This should be less prone to bugs.

@tkelman
Copy link
Contributor

tkelman commented May 27, 2017

This should be less prone to bugs.

How so?

@TotalVerb
Copy link
Contributor Author

big number constant literals might be incorrect; it takes some work to verify. of course I think the original literals have been verified already, but there isn't any harm in doing this.

@tkelman
Copy link
Contributor

tkelman commented May 27, 2017

what's more likely to sneak through without tests noticing, the literals being wrong (or mis-parsed?) or something screwy happening to the generation code like an accidental overflow due to some other problem?

@TotalVerb
Copy link
Contributor Author

TotalVerb commented May 27, 2017

Any test that detects the literal being wrong will certainly also detect the generation code that generates its equivalent being wrong.

@TotalVerb
Copy link
Contributor Author

Also, it is easier for a human to detect an error in the generation code than to detect an error in the literal.

@tkelman
Copy link
Contributor

tkelman commented May 27, 2017

Probably, and it's a bit shorter. We might want to preallocate them to the right size, to avoid a few bytes of overallocation that'll never be needed.

@GregPlowman
Copy link
Contributor

Maybe the existing literal tables could be used as part of the tests for the generated values?

@ViralBShah
Copy link
Member

How about the other way around - Why not leave the literal tables here, and instead have the code verify the tables in the tests? Does this have any noticeable impact on startup time (just asking)?

@Keno
Copy link
Member

Keno commented May 28, 2017

Does this have any noticeable impact on startup time (just asking)?

This would happen at sysimg time, so any time impact would only be there. This shouldn't take any noticeable amount of time though.

@kshyatt kshyatt added the maths Mathematical functions label May 29, 2017
@TotalVerb
Copy link
Contributor Author

I've addressed the comments of @tkelman and @GregPlowman.

@KristofferC KristofferC merged commit 9b9c93b into JuliaLang:master May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants