Skip to content

round: Bypass the precision logic when rounding to 0 places #12284

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimWolla
Copy link
Member

Since GH-12220 the implementation of php_round_helper(), which performs rounding to an integral value, is easy to verify for correctness up to the floating point precision.

If rounding to 0 places is desired, i.e. the userland round() function is called with $precision = 0, we bypass all logic for the decimal point adjustment and instead directly call php_round_helper().

This change fixes the remaining two cases of GH-12143 and likely guarantees correct rounding for all possible inputs and $precision = 0.

/cc @SakiTakamachi

Since phpGH-12220 the implementation of `php_round_helper()`, which performs
rounding to an integral value, is easy to verify for correctness up to the
floating point precision.

If rounding to 0 places is desired, i.e. the userland `round()` function is
called with `$precision = 0`, we bypass all logic for the decimal point
adjustment and instead directly call `php_round_helper()`.

This change fixes the remaining two cases of phpGH-12143 and likely guarantees
correct rounding for all possible inputs and `$precision = 0`.
@SakiTakamachi
Copy link
Member

I haven't tried it yet, so this is just a prediction, but I think the results will be incorrect in the following cases.

round(1.700000000000145, 13, PHP_ROUND_HALF_UP);
round(1.550000000000001, 1, PHP_ROUND_HALF_DOWN)

@TimWolla
Copy link
Member Author

but I think the results will be incorrect in the following cases.

These cases are not handled by this PR. It only handles the case where the second parameter is 0, i.e. integer rounding is desired. This is the case we can do exactly up to floating point precision.

@SakiTakamachi
Copy link
Member

Ah, I understand.

I cannot agree to this change.
Is there really any point in changing only the behavior when precision is 0?

Rounding to the 16th digit is not included as it is a functional modification, but all other cases can be fixed with my changes.
(There may be room for discussion on how to handle edge cases, but it would be a good idea to make some changes depending on the discussion.)

#12268

And bypassing processing when precision is 0 as a way to make the 16th digit possible is completely wrong.

@Girgias
Copy link
Member

Girgias commented Sep 23, 2023

This looks sensible? But I must say the precision adjustment code is quite confusing (probably due to the extremely generic variable names...).

But overhaul, shouldn't possible issues only be present when using strictly positive precisions? And it seems like it tries to handle those before delegating to the round helper. (Point being I don't totally understand why negative precisions, which will round to an integer value, seems to use the same logic as something rounding to a float val).

@bukka
Copy link
Member

bukka commented Oct 13, 2023

I would prefer to see more complete solution to handle all cases which is either #12268 or #12291 .

@jorgsowa
Copy link
Contributor

jorgsowa commented Sep 2, 2024

Hi @TimWolla, will you continue this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants