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

Fix int_fast16_t overflow #3740

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Fix int_fast16_t overflow #3740

merged 2 commits into from
Dec 21, 2021

Conversation

ramon-bernardo
Copy link
Contributor

Pull Request Prelude

Changes Proposed

  • Fix int_fast16_t overflow

soul4soul
soul4soul previously approved these changes Oct 16, 2021
@DSpeichert
Copy link
Member

From the commit message, PR message and the code, I still have no idea what the problem is and what the fix fixes.

@ranisalt
Copy link
Member

ranisalt commented Oct 16, 2021

From the commit message, PR message and the code, I still have no idea what the problem is and what the fix fixes.

The range arguments are int32, but the result variable is int16, therefore overflow may occur if the range does not fit 16 bits. It doesn't usually happen, because the range is usually small (this function is heavy af, it usually runs with the viewport dimensions), but the conversion MAY hurt performance.

I would just use int everywhere.

@DSpeichert
Copy link
Member

@ranisalt int or int32_t to keep it the same?

@ranisalt
Copy link
Member

ranisalt commented Dec 16, 2021

auto to avoid conversions, but int32_t between those (also avoids conversion)

@DSpeichert DSpeichert merged commit 17bf638 into otland:master Dec 21, 2021
@ramon-bernardo ramon-bernardo deleted the i16-overflow branch December 21, 2021 23:06
Znote pushed a commit to Znote/forgottenserver that referenced this pull request Jan 30, 2022
DSpeichert pushed a commit that referenced this pull request May 9, 2022
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.

None yet

5 participants