Fix #12143: Improve precision of rounding for FP numbers by jorgsowa · Pull Request #12159 · php/php-src

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes#12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include<math.h>#include<float.h>#include<stdio.h>#ifndefPHP_ROUND_HALF_UP#definePHP_ROUND_HALF_UP 0x01 /* Arithmetic rounding, up == away from zero */#endif#ifndefPHP_ROUND_HALF_DOWN#definePHP_ROUND_HALF_DOWN 0x02 /* Down == towards zero */#endif#ifndefPHP_ROUND_HALF_EVEN#definePHP_ROUND_HALF_EVEN 0x03 /* Banker's rounding */#endif#ifndefPHP_ROUND_HALF_ODD#definePHP_ROUND_HALF_ODD 0x04
#endifdoublephp_round_helper(doublevalue, intmode) {
doubletmp_value;
if (value >= 0.0) {
tmp_value=floor(value+0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (-0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (0.5+2*floor(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (0.5+2*floor(tmp_value/2.0) -1.0)))
{
tmp_value=tmp_value-1.0;
}
} else {
tmp_value=ceil(value-0.5);
if ((mode==PHP_ROUND_HALF_DOWN&&value== (0.5+tmp_value)) ||
(mode==PHP_ROUND_HALF_EVEN&&value== (-0.5+2*ceil(tmp_value/2.0))) ||
(mode==PHP_ROUND_HALF_ODD&&value== (-0.5+2*ceil(tmp_value/2.0) +1.0)))
{
tmp_value=tmp_value+1.0;
}
}
returntmp_value;
}
intmain(void) {
doubled;
doublevalues[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };
for (size_ti=0; i<sizeof(values) / sizeof(values[0]); i++) {
doublevalue=values[i];
d=nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
d=nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
printf("\n");
}
}

SakiTakamachi · 2023-09-12T00:59:32Z

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]
The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:
3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001
Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):
Suggested change
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.
[FYI]
These internal representations and their corresponding FP are:
3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002
4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) ClosesphpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) ClosesGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

Change precision of rounding for large FP numbers:
6c474eb

github-actionsBot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND
efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …
112d9d4
…PHP_ROUND_HALF_EVEN

Removed changes to prerounding
f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~Fix #12123: Improve precision of rounding for FP numbersSep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023
Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5#12143
Closed

TimWolla reviewed Sep 9, 2023
View reviewed changes

Added more numbers to test
f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~Fix #12143: Improve precision of rounding for FP numbersSep 10, 2023

SakiTakamachi reviewed Sep 12, 2023
View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023
Reimplement php_round_helper() using modf()#12220
Merged

TimWolla closed this in #12220 Sep 19, 2023

jorgsowa deleted the fix-gh-12143 branch July 10, 2026 14:30

-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

Uh oh!

Conversation

jorgsowa commented Sep 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Sep 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimWolla commented Sep 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Sep 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Sep 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jorgsowa commented Sep 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Sep 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimWolla commented Sep 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Sep 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Sep 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jorgsowa commented Sep 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Sep 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

jorgsowa commented Sep 8, 2023•
edited
Loading

SakiTakamachi commented Sep 9, 2023•
edited
Loading

TimWolla commented Sep 10, 2023•
edited
Loading

bukka commented Sep 15, 2023•
edited
Loading

SakiTakamachi commented Sep 15, 2023•
edited
Loading

jorgsowa commented Sep 8, 2023•
edited
Loading

SakiTakamachi commented Sep 9, 2023•
edited
Loading

TimWolla commented Sep 10, 2023•
edited
Loading

bukka commented Sep 15, 2023•
edited
Loading

SakiTakamachi commented Sep 15, 2023•
edited
Loading

jorgsowa commented Sep 8, 2023•
edited
Loading

SakiTakamachi commented Sep 9, 2023•
edited
Loading

TimWolla commented Sep 10, 2023•
edited
Loading

bukka commented Sep 15, 2023•
edited
Loading

SakiTakamachi commented Sep 15, 2023•
edited
Loading

jorgsowa commented Sep 8, 2023•
edited
Loading

SakiTakamachi commented Sep 9, 2023•
edited
Loading

TimWolla commented Sep 10, 2023•
edited
Loading