Skip to content

RFC: Integer rounding #12301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

RFC: Integer rounding #12301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db98d1d
ceil/floor/round keeps given int type if possible
marc-mabe Jun 14, 2023
2483a75
UNEXPECTED conditions
marc-mabe Sep 26, 2023
f9ae196
Update link to RFC
marc-mabe Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3069,13 +3069,13 @@ function mail(string $to, string $subject, string $message, array|string $additi
function abs(int|float $num): int|float {}

/** @compile-time-eval */
function ceil(int|float $num): float {}
function ceil(int|float $num): int|float {}

/** @compile-time-eval */
function floor(int|float $num): float {}
function floor(int|float $num): int|float {}
Comment on lines 3071 to +3075
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point to keep float return type just for 32bit builds?

Also, $num to be of type int doesn't make any sense, as the function is a no-op in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... that's already a bug in the stubs.
This needs to be backported to 8.1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point to keep float return type just for 32bit builds?

flooring 2.5e300 should be legal and the result is only representable as float.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RFC proposes to keep the given type if possible.

  • float given -> float returned (guarantied)
  • int given -> int returned (except for int overflow on round with precision < 0)


/** @compile-time-eval */
function round(int|float $num, int $precision = 0, int $mode = PHP_ROUND_HALF_UP): float {}
function round(int|float $num, int $precision = 0, int $mode = PHP_ROUND_HALF_UP): int|float {}

/** @compile-time-eval */
function sin(float $num): float {}
Expand Down
10 changes: 4 additions & 6 deletions ext/standard/basic_functions_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

115 changes: 109 additions & 6 deletions ext/standard/math.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ static inline double php_round_helper(double value, int mode) {

/* {{{ _php_math_round */
/*
* Rounds a number to a certain number of decimal places in a certain rounding
* Rounds a floating point to a certain number of decimal places in a certain rounding
* mode. For the specifics of the algorithm, see http://wiki.php.net/rfc/rounding
*/
PHPAPI double _php_math_round(double value, int places, int mode) {
Expand Down Expand Up @@ -249,6 +249,108 @@ PHPAPI double _php_math_round(double value, int places, int mode) {
}
/* }}} */

/* {{{ _php_math_round_long */
/*
* Rounds a zend_long to a certain number of decimal places in a certain rounding
* mode. For the specifics of the algorithm, see https://wiki.php.net/rfc/integer-rounding
*/
PHPAPI zend_result _php_math_round_long(zend_long value, int places, int mode, zend_long *result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly I find this implementation incomprehensible. I've specifically refactored the floating point implementation to leverage a switch statement with very simple logic to make the code easier to verify for correctness in #12220.

This pattern should likely also be applied here. It also doesn't help that the logic is effectively duplicated for the “above zero” and “below zero” cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tim,
This is a first working version actually started before #12220 and as such I was following the previous style.
But you are right this can and should be improved. If the RFC gets accepted I'll update the PR according.

static const zend_long powers[] = {
1, 10, 100, 1000, 10000,
100000, 1000000, 10000000, 100000000, 1000000000,
#if SIZEOF_ZEND_LONG >= 8
10000000000, 100000000000, 1000000000000, 10000000000000, 100000000000000,
1000000000000000, 10000000000000000, 100000000000000000, 1000000000000000000
#elif SIZEOF_ZEND_LONG > 8
# error "Unknown SIZEOF_ZEND_LONG"
#endif
};

zend_long tmp_value = value;
zend_long power;
zend_long power_half;
zend_long rest;

// Boolean if the number of places used matches the number of places possible
int max_places = 0;

// Simple case - long that does not need to be rounded
if (places >= 0) {
*result = tmp_value;
return SUCCESS;
}

if (UNEXPECTED(-places > sizeof(powers) / sizeof(powers[0]) - 1)) {
// Special case for rounding to the same number of places as max length possible
// as this would overflow the power of 10
if (places == -MAX_LENGTH_OF_LONG + 1) {
max_places = 1;
rest = tmp_value;
tmp_value = 0;
power_half = powers[-places - 1] * 5;
} else {
// Rounding more places will always be zero
*result = 0;
return SUCCESS;
}
} else {
power = powers[-places];
rest = tmp_value % power;
power_half = power / 2;
tmp_value = tmp_value / power;
}

if (value >= 0) {
if ((mode == PHP_ROUND_HALF_UP && rest >= power_half)
|| (mode == PHP_ROUND_HALF_DOWN && rest > power_half)
|| (mode == PHP_ROUND_HALF_EVEN && (rest > power_half || (rest == power_half && tmp_value % 2 == 1)))
|| (mode == PHP_ROUND_HALF_ODD && (rest > power_half || (rest == power_half && tmp_value % 2 == 0)))
) {
if (UNEXPECTED(max_places)) {
return FAILURE; // would overflow
}

tmp_value = tmp_value * power;

if (UNEXPECTED(tmp_value > ZEND_LONG_MAX - power)) {
return FAILURE; // would overflow
}
tmp_value = tmp_value + power;
} else if (UNEXPECTED(max_places)) {
tmp_value = 0;
} else {
tmp_value = tmp_value * power;
}
} else {
if ((mode == PHP_ROUND_HALF_UP && rest <= -power_half)
|| (mode == PHP_ROUND_HALF_DOWN && rest < -power_half)
|| (mode == PHP_ROUND_HALF_EVEN && (rest < -power_half || (rest == -power_half && tmp_value % 2 == -1)))
|| (mode == PHP_ROUND_HALF_ODD && (rest < -power_half || (rest == -power_half && tmp_value % 2 == 0)))
) {
if (UNEXPECTED(max_places)) {
return FAILURE; // would underflow
}

tmp_value = tmp_value * power;

if (UNEXPECTED(tmp_value < ZEND_LONG_MIN + power)) {
return FAILURE; // would underflow
}

tmp_value = tmp_value - power;
} else if (UNEXPECTED(max_places)) {
tmp_value = 0;
} else {
tmp_value = tmp_value * power;
}
}

*result = tmp_value;
return SUCCESS;
}
/* }}} */


/* {{{ Return the absolute value of the number */
PHP_FUNCTION(abs)
{
Expand Down Expand Up @@ -283,7 +385,7 @@ PHP_FUNCTION(ceil)

switch (Z_TYPE_P(value)) {
case IS_LONG:
RETURN_DOUBLE(zval_get_double(value));
RETURN_ZVAL(value, 1, 0);
case IS_DOUBLE:
RETURN_DOUBLE(ceil(Z_DVAL_P(value)));
EMPTY_SWITCH_DEFAULT_CASE();
Expand All @@ -302,7 +404,7 @@ PHP_FUNCTION(floor)

switch (Z_TYPE_P(value)) {
case IS_LONG:
RETURN_DOUBLE(zval_get_double(value));
RETURN_ZVAL(value, 1, 0);
case IS_DOUBLE:
RETURN_DOUBLE(floor(Z_DVAL_P(value)));
EMPTY_SWITCH_DEFAULT_CASE();
Expand All @@ -317,6 +419,7 @@ PHP_FUNCTION(round)
int places = 0;
zend_long precision = 0;
zend_long mode = PHP_ROUND_HALF_UP;
zend_long return_val_l;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use preferable some better name rather than something ending with just l (e.g. something like return_long_val or return_lval)


ZEND_PARSE_PARAMETERS_START(1, 3)
Z_PARAM_NUMBER(value)
Expand Down Expand Up @@ -346,10 +449,10 @@ PHP_FUNCTION(round)

switch (Z_TYPE_P(value)) {
case IS_LONG:
/* Simple case - long that doesn't need to be rounded. */
if (places >= 0) {
RETURN_DOUBLE(zval_get_double(value));
if (_php_math_round_long(Z_LVAL_P(value), places, (int)mode, &return_val_l) == SUCCESS) {
RETURN_LONG(return_val_l);
}

ZEND_FALLTHROUGH;

case IS_DOUBLE:
Expand Down
1 change: 1 addition & 0 deletions ext/standard/php_math.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define PHP_MATH_H

PHPAPI double _php_math_round(double value, int places, int mode);
PHPAPI zend_result _php_math_round_long(zend_long value, int places, int mode, zend_long *result);
PHPAPI zend_string *_php_math_number_format(double d, int dec, char dec_point, char thousand_sep);
PHPAPI zend_string *_php_math_number_format_ex(double d, int dec, const char *dec_point, size_t dec_point_len, const char *thousand_sep, size_t thousand_sep_len);
PHPAPI zend_string *_php_math_number_format_long(zend_long num, zend_long dec, const char *dec_point, size_t dec_point_len, const char *thousand_sep, size_t thousand_sep_len);
Expand Down
22 changes: 15 additions & 7 deletions ext/standard/tests/math/ceil_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ precision=14
echo "*** Testing ceil() : basic functionality ***\n";
$values = array(0,
-0,
0.0,
-0.0,
0.5,
-0.5,
1,
-1,
1.0,
-1.0,
1.5,
-1.5,
2.6,
Expand All @@ -35,25 +39,29 @@ for ($i = 0; $i < count($values); $i++) {
?>
--EXPECTF--
*** Testing ceil() : basic functionality ***
int(0)
int(0)
float(0)
float(0)
float(-0)
float(1)
float(-0)
int(1)
int(-1)
float(1)
float(-1)
float(2)
float(-1)
float(3)
float(-2)
float(31)
float(95)
int(31)
int(95)
float(11)
float(-10)
float(3950)
float(-3950)
float(39)
float(1)
float(0)
int(39)
int(1)
int(0)

Deprecated: ceil(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
float(0)
int(0)
26 changes: 13 additions & 13 deletions ext/standard/tests/math/ceil_basiclong_64bit.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,32 @@ foreach ($longVals as $longVal) {
?>
--EXPECT--
--- testing: 9223372036854775807 ---
float(9.223372036854776E+18)
int(9223372036854775807)
--- testing: -9223372036854775808 ---
float(-9.223372036854776E+18)
int(-9223372036854775808)
--- testing: 2147483647 ---
float(2147483647)
int(2147483647)
--- testing: -2147483648 ---
float(-2147483648)
int(-2147483648)
--- testing: 9223372034707292160 ---
float(9.223372034707292E+18)
int(9223372034707292160)
--- testing: -9223372034707292160 ---
float(-9.223372034707292E+18)
int(-9223372034707292160)
--- testing: 2147483648 ---
float(2147483648)
int(2147483648)
--- testing: -2147483649 ---
float(-2147483649)
int(-2147483649)
--- testing: 4294967294 ---
float(4294967294)
int(4294967294)
--- testing: 4294967295 ---
float(4294967295)
int(4294967295)
--- testing: 4294967293 ---
float(4294967293)
int(4294967293)
--- testing: 9223372036854775806 ---
float(9.223372036854776E+18)
int(9223372036854775806)
--- testing: 9.2233720368548E+18 ---
float(9.223372036854776E+18)
--- testing: -9223372036854775807 ---
float(-9.223372036854776E+18)
int(-9223372036854775807)
--- testing: -9.2233720368548E+18 ---
float(-9.223372036854776E+18)
16 changes: 8 additions & 8 deletions ext/standard/tests/math/ceil_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,24 +77,24 @@ fclose($fp);
-- Iteration 1 --

Deprecated: ceil(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
float(0)
int(0)

-- Iteration 2 --

Deprecated: ceil(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
float(0)
int(0)

-- Iteration 3 --
float(1)
int(1)

-- Iteration 4 --
float(0)
int(0)

-- Iteration 5 --
float(1)
int(1)

-- Iteration 6 --
float(0)
int(0)

-- Iteration 7 --
ceil(): Argument #1 ($num) must be of type int|float, string given
Expand All @@ -120,12 +120,12 @@ ceil(): Argument #1 ($num) must be of type int|float, classA given
-- Iteration 14 --

Deprecated: ceil(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
float(0)
int(0)

-- Iteration 15 --

Deprecated: ceil(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
float(0)
int(0)

-- Iteration 16 --
ceil(): Argument #1 ($num) must be of type int|float, resource given
Loading