Fix wrong integer division where rounding up was intended #1254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simogasp
reviewed
Oct 2, 2022
p12tic
force-pushed
the
fix-int-division-ceil
branch
from
8bb2785 to
126b41a
Compare
simogasp
reviewed
Oct 2, 2022
p12tic
force-pushed
the
fix-int-division-ceil
branch
2 times, most recently
from
1dadfe4 to
2103e16
Compare
|
I've updated the function again. I managed to leave an error for negative numbers and ran missed it by running tests that haven't yet been recompiled with updated code. |
simogasp
reviewed
Oct 2, 2022
p12tic
force-pushed
the
fix-int-division-ceil
branch
from
2103e16 to
51e6f6a
Compare
|
it's funny how an apparently simple function like this still has so many entries on StackOverflow and many different answers. |
simogasp
previously approved these changes
Oct 2, 2022
simogasp
reviewed
Oct 2, 2022
simogasp
reviewed
Oct 2, 2022
p12tic
force-pushed
the
fix-int-division-ceil
branch
from
51e6f6a to
87c6537
Compare
fabiencastan
reviewed
Oct 6, 2022
The intention here likely was to get floating-point division result and round it upwards. However, the division was performed with integer arguments, fractional part was lost and thus std::ceil did nothing.
p12tic
force-pushed
the
fix-int-division-ceil
branch
from
87c6537 to
2caea25
Compare
fabiencastan
approved these changes
Oct 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The intention in cases like
std::ceil(a / b)(whereaandbare integers) likely was to get floating-point division result and round it upwards. However, the division was performed with integer arguments, fractional part was lost and thusstd::ceildid nothing. This PR introduces a separate function to perform such integer division and fixes the buggy cases.If the original intention was wrong, this PR will break behavior. According to my limited understanding of the code the original intention was correct.
A subsequent PR will change all similar cases where we currently do
int(std::ceil(double(a) / double(b)))to usedivideRoundUp().