Hi all,
The "round" function supports rounding to significant digits before the
decimal point which can be very helpful
but unfortunately it's handling everything as floating point numbers
which in the end can lead to inaccurate results.
printf("%d\n", round(987654321098765432, -1) // 987654321098765440
instead of 987654321098765430
To be fair it's also documented as such:
round — Rounds a float
round(int|float $num, int $precision = 0, int $mode =
PHP_ROUND_HALF_UP): float
I would like to propose to change this function to keep the given type
and proper handle integers as well:
round — Rounds a number
/** @template T of int|float */
round(T $num, int $precision = 0, int $mode = PHP_ROUND_HALF_UP): T
Do you think this could be an acceptable BC-break or should this be a
different function?
Thanks,
Marc
Do you think this could be an acceptable BC-break
No. Suggesting changing a 30 year old maths operations is a huge BC break.
or should this be a different function?
Just make your own that does precisely what you want...
cheers
Dan
Ack
Do you think this could be an acceptable BC-break
No. Suggesting changing a 30 year old maths operations is a huge BC break.
or should this be a different function?
Just make your own that does precisely what you want...
I agree on the first point, but disagree on the second. It's far too late to change round() itself, but the lack of built-in functions for converting to int in a controlled way is frustrating, particularly as the fashion moves towards stronger typing in general.
On the surface, it sounds like a trivial operation, but there's a lot of edge cases to think about (limits of FP precision, negatives including negative zero, etc), and shipping a robust implementation of int_round, int_floor and int_ceil in core would save everyone having to rediscover them the hard way.
Regards,
--
Rowan Tommins
[IMSoP]
Do you think this could be an acceptable BC-break
No. Suggesting changing a 30 year old maths operations is a huge BC break.
or should this be a different function?
Just make your own that does precisely what you want...
I agree on the first point, but disagree on the second. It's far too
late to changeround()itself, but the lack of built-in functions for
converting to int in a controlled way is frustrating, particularly as
the fashion moves towards stronger typing in general.On the surface, it sounds like a trivial operation, but there's a lot
of edge cases to think about (limits of FP precision, negatives
including negative zero, etc), and shipping a robust implementation of
int_round, int_floor and int_ceil in core would save everyone having to
rediscover them the hard way.
Having recently been bitten by floor() returning a float even though the value that comes back is logically an int, I would be fully in support of "and returns an int" versions of these functions in core.
--Larry Garfield
Do you think this could be an acceptable BC-break
No. Suggesting changing a 30 year old maths operations is a huge BC break.or should this be a different function?
Just make your own that does precisely what you want...
I agree on the first point, but disagree on the second. It's far too
late to changeround()itself, but the lack of built-in functions for
converting to int in a controlled way is frustrating, particularly as
the fashion moves towards stronger typing in general.On the surface, it sounds like a trivial operation, but there's a lot
of edge cases to think about (limits of FP precision, negatives
including negative zero, etc), and shipping a robust implementation of
int_round, int_floor and int_ceil in core would save everyone having to
rediscover them the hard way.
Having recently been bitten byfloor()returning a float even though the value that comes back is logically an int, I would be fully in support of "and returns an int" versions of these functions in core.
Yes, ceil and floor have the same issue and probably some others as
well. I remember number_format expecting float only.
About changing the behavior of such an old method I also have mixed
feelings about. On the one hand handling a number afterwards still
wouldn't change but with more and more type safety it's probably
breaking more then expected.
A possible way could be to trigger some kind of warning if
round/floor/ceil gets called with an integer but I don't see a good fit
here as it's not a deprecation and notice/warning also does not make
sense here I think.
New function(s) on the other hand sounds like an ugly solution as well
only helping people explicitly searching for it after they got bitten at
least once.
Scalar object methods, yes that would help in faaar future (only once
until first release).
Just thinking out loud.
Marc
New function(s) on the other hand sounds like an ugly solution as well
only helping people explicitly searching for it after they got bitten at
least once.
What about deprecating round() in favor of round_float() and
round_int()? or something.
Robert Landers
Software Engineer
Utrecht NL
Hi again,
New function(s) on the other hand sounds like an ugly solution as well
only helping people explicitly searching for it after they got bitten at
least once.
What about deprecatinground()in favor of round_float() and
round_int()? or something.Robert Landers
Software Engineer
Utrecht NL
I have done some research and worked on an implementation [1] even
thought I'm not a C engineer.
There are some things I have noticed:
- From PHP's type system perspective an int is completely compatible to
an int also in strict mode - https://3v4l.org/sspOM
That means making round, ceil and floor returning an int should not be
much less of an issue
- The functions round, ceil and floor are documented to take an
int|float as argument rather then a float only instead of a float only
For me that means there is already some kind of difference expected as
if it would be documented as float only
-
Documenting it as input type matches the output type does not work in
corner cases as rounding up/down but integer numbers could lead to
integer over/under flows which needs to be handled and casted into a
floating point number falling back current behavior. -
I figured out one more function "number_format()" which casts
everything into a floating point number even though it can unnecessary
ending up in rounding errors.
Fixing this is not even a BC break at all and could go separately as a
bug fix in my opinion.
- It least in case of ceil and floor this would be a no-op instead of a
cast + memory allocation for a new zval
In case of round as well but there are more things to do - haven't done
any performance tests so far
I have the felling that this change is not that much of a BC break and
serves the right purpose by reducing rounding errors as much as possible.
Still as these are very highly used functions I would target the next
major just as there BC breaks are somehow expected even though I now
think the only BC break is on explicitly checking the types after rounding.
Thanks,
Marc
[1]
https://github.com/php/php-src/compare/master...marc-mabe:php-src:integer_rounding
On Sun, May 21, 2023 at 4:21 PM Larry Garfield larry@garfieldtech.com
wrote:
Having recently been bitten by
floor()returning a float even though the
value that comes back is logically an int, I would be fully in support of
"and returns an int" versions of these functions in core.
What about adding a third, optional parameter to round() which is a flag
(or bitmask) to specify preferred behaviour and return type? Kind of ugly,
but maybe less ugly than another new function and no BC-break, as the
default can be the existing behaviour.
-Dave