Hi all,
there has been a "small" change in the rounding logic of
_php_math_round()[1] a couple of months ago. A respective ticket,
claiming the new behavior would be a bug[2] had been filed a while ago
without proper triage so far. I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet). At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.
My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things. This change apparently
improves some cases, but worsens others. Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.
What do you think?
[1] https://github.com/php/php-src/pull/12222
[2] https://github.com/php/php-src/issues/14332
Cheers,
Christoph
Hi Christoph,
Hi all,
there has been a "small" change in the rounding logic of
_php_math_round()[1] a couple of months ago. A respective ticket,
claiming the new behavior would be a bug[2] had been filed a while ago
without proper triage so far. I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet). At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things. This change apparently
improves some cases, but worsens others. Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.What do you think?
[1] https://github.com/php/php-src/pull/12222
[2] https://github.com/php/php-src/issues/14332Cheers,
Christoph
As you know, I am the implementer of that change.
At the time, I thought "just add another digit," but this may have been a more disruptive change than I realized.
I'm not opposed to reverting this, but I don't have any strong opinions on whether it should be reverted.
I'd like to hear other people's opinions as well.
Regards,
Saki
Hi all,
there has been a "small" change in the rounding logic of
_php_math_round()[1] a couple of months ago. A respective ticket,
claiming the new behavior would be a bug[2] had been filed a while ago
without proper triage so far. I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet). At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things. This change apparently
improves some cases, but worsens others. Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.What do you think?
[1] https://github.com/php/php-src/pull/12222
[2] https://github.com/php/php-src/issues/14332Cheers,
Christoph
I guess the real question is: how many of these numbers should be the same?
https://3v4l.org/tgRMc/rfc#vgit.master
— Rob
Hi all,
there has been a "small" change in the rounding logic of
_php_math_round()[1] a couple of months ago. A respective ticket,
claiming the new behavior would be a bug[2] had been filed a while ago
without proper triage so far. I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet). At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things. This change apparently
improves some cases, but worsens others. Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.What do you think?
I agree, this situation is extremely suboptimal.
My understanding as to why people declined the "Change the edge case of round()" RFC [1] was because they were worried about the silent change in behaviour.
However, most people seem to have missed the fact, myself included, that regardless of it being accepted or not there would have been changes to the behaviour.
As such we have the worst of both worlds, continuing to have incorrect floating point semantics for people that rely on proper IEEE 754 floating points,
and yet we still have a silent change in behaviour due to the partial bugfix.
Moreover, these fixes made the implementation of round() more complicated, for marginal benefits IMHO.
Best regards,
Gina P. Banyard
Hi Gina,
I agree, this situation is extremely suboptimal.
My understanding as to why people declined the "Change the edge case of
round()" RFC [1] was because they were worried about the silent change in behaviour.
However, most people seem to have missed the fact, myself included, that regardless of it being accepted or not there would have been changes to the behaviour.
As such we have the worst of both worlds, continuing to have incorrect floating point semantics for people that rely on proper IEEE 754 floating points,
and yet we still have a silent change in behaviour due to the partial bugfix.Moreover, these fixes made the implementation of
round()more complicated, for marginal benefits IMHO.Best regards,
Gina P. Banyard
Opinion on this seems to be more divided than I expected.
I think the fairest approach would be to revert the change that extended the range of rounding to one more digit and hold a RFC vote.
Note that it doesn't take 2/3 to take down this change, it takes 2/3 to keep this change.
How do you think?
Regards,
Saki
Le 12 juil. 2024 à 13:24, Christoph M. Becker cmbecker69@gmx.de a écrit :
Hi all,
there has been a "small" change in the rounding logic of
_php_math_round()1 a couple of months ago. A respective ticket,
claiming the new behavior would be a bug2 had been filed a while ago
without proper triage so far. I'm bringing this to your attention,
because I'm afraid that there will be (many) more bug reports about this
in the future (note that the change only affects master, and only PHP
8.4.0alpha1 has been released yet, so it is probably not widespreadly
tested yet). At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.My personal stance on this is simple: floating point arithmetic is not
exact per se, and changes to how PHP handles the details should only be
introduced, if they clearly improve things. This change apparently
improves some cases, but worsens others. Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.What do you think?
1 https://github.com/php/php-src/pull/12222
2 https://github.com/php/php-src/issues/14332Cheers,
Christoph
Hi,
See 1 and 2, which motivated the change.
I expect that round($anything) == (int) round($anything) for any float between PHP_INT_MIN and PHP_INT_MAX, and I think that the change fixed that .
Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).
—Claude
Hi!
Le 12 juil. 2024 à 13:24, Christoph M. Becker cmbecker69@gmx.de a écrit :
[…] At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.[…] Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.
Ah, thank you! I probably should have checked this more thouroughly;
now even I can see that there was a bug, so it is okay for me to stick
with the fix (thank you, Saki!), but maybe UPGRADING should clarify the
issue a bit. Currently, it states:
| The maximum precision that can be handled by round() has been extended
| by one digit.
While that is technically correct, probably few readers understand the
implications.
I expect that
round($anything) == (int) round($anything)for any float betweenPHP_INT_MINand PHP_INT_MAX, and I think that the change fixed that .
It seems to me that this is a reasonable expectation.
Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).
Makes sense (unless there is already such a test, of course).
Cheers,
Christoph
Hi Christoph,
Hi!
Le 12 juil. 2024 à 13:24, Christoph M. Becker cmbecker69@gmx.de a écrit :
[…] At the very least we should be sure that we want to keep
this change, and document it well, to avoid discussions in every filed
ticket.[…] Thus, I think the change
should better be reverted, and if at all, postponed to PHP 9, since I
neither regard this change as bug fix nor as a feature.Ah, thank you! I probably should have checked this more thouroughly;
now even I can see that there was a bug, so it is okay for me to stick
with the fix (thank you, Saki!), but maybe UPGRADING should clarify the
issue a bit. Currently, it states:| The maximum precision that can be handled by
round()has been extended
| by one digit.While that is technically correct, probably few readers understand the
implications.I expect that
round($anything) == (int) round($anything)for any float betweenPHP_INT_MINand PHP_INT_MAX, and I think that the change fixed that .It seems to me that this is a reasonable expectation.
Therefore, I am for keeping the change, and ensuring that there are test cases for 4503599627370495.5 (the largest float with fractional part).
Makes sense (unless there is already such a test, of course).
Cheers,
Christoph
If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)
I will update UPGRADING with concrete examples.
Regards,
Saki
Hi Saki!
Ah, thank you! I probably should have checked this more thouroughly;
now even I can see that there was a bug, so it is okay for me to stick
with the fix (thank you, Saki!), […]If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)
Well, if I call round() and tell it to round to zero decimals, and it
doesn't do it (assuming precision=-1), that looks like a bug to me.
I will update UPGRADING with concrete examples.
Thank you!
Cheers,
Christoph
Hi Christoph,
2024/07/13 22:30、Christoph M. Becker cmbecker69@gmx.deのメール:
Hi Saki!
Ah, thank you! I probably should have checked this more thouroughly;
now even I can see that there was a bug, so it is okay for me to stick
with the fix (thank you, Saki!), […]If this can be considered a bug fix, then I'm in favor of keeping it as is. (I wasn't sure if this should be considered a feature addition.)
Well, if I call
round()and tell it to round to zero decimals, and it
doesn't do it (assuming precision=-1), that looks like a bug to me.I will update UPGRADING with concrete examples.
Thank you!
Cheers,
Christoph
I opened the PR.
https://github.com/php/php-src/pull/14943
FYI, the test case for that value already exists :)
https://github.com/php/php-src/blob/520787bb93990b95d9cf6eaf063ca2b03b76a87f/ext/standard/tests/math/round_gh12143_expand-rounding-target.phpt#L14
Regards,
Saki