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

Draft: Possibly some safer arithmetics #137

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

Nicceboy
Copy link
Contributor

@Nicceboy Nicceboy commented Aug 7, 2023

Hey,

I ran the Facebook's MIRAI static analysis tool for this project, and it identified couple issues which might be real or not.

I added boundary checks for some arithmetics as a result, which looked like potential problem scenarios. I do not understand to code well enough to say for sure. Or should we create tests for these?

Adding overflow check might come with with small performance penalties. So it might be good to look if some are indeed relevant.

@XAMPPRocky
Copy link
Collaborator

XAMPPRocky commented Aug 8, 2023

Thank you for your PR!

So it might be good to look if some are indeed relevant.

I'd rather do the opposite. Add the checks now, and then have someone make a followup PR showing that it's impossible and removing them and showing how much of a performance boost it provides, we don't have benchmarking tooling in CI yet.

@XAMPPRocky XAMPPRocky merged commit f052688 into librasn:main Aug 8, 2023
68 checks passed
@github-actions github-actions bot mentioned this pull request Aug 8, 2023
@Nicceboy Nicceboy deleted the some-safer-arithmetics branch August 11, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants