round: Bypass the precision logic when rounding to 0 places by TimWolla · Pull Request #12284 · php/php-src

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla · 2023-09-23T14:27:21Z

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 · 2023-09-23T14:34:54Z

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 · 2023-09-23T14:38:34Z

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 · 2023-09-23T14:58:26Z

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 · 2023-09-23T15:00:19Z

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 · 2023-10-13T10:28:54Z

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

jorgsowa · 2024-09-02T16:11:57Z

Hi @TimWolla, will you continue this PR?

Girgias · 2026-04-06T17:43:38Z

I don't think this PR is still relevant considering the various round() RFCs. Can this be closed @TimWolla ?

TimWolla requested a review from bukka as a code owner September 23, 2023 14:27