RFC: Add 4 new rounding modes to round() function #12056

jorgsowa · 2023-08-26T20:30:32Z

This PR introduces fours new modes to the round() function: PHP_ROUND_CEILING, PHP_ROUND_FLOOR, PHP_ROUND_AWAY_FROM_ZERO and PHP_ROUND_TOWARD_ZERO.

Those four modes are frequently used in accounting and are heavily requested by the PHP users. Two first comments in round() documentation page is about this feature and how can it currently be achieved in the userland. However if we already have 4 existing modes it's not big effort to implement next four modes which are more popular than existing PHP_ROUND_HALF_EVEN and PHP_ROUND_HALF_ODD and in fact completing all of the most popular rounding modes.

The RFC for the change has been approved: https://wiki.php.net/rfc/new_rounding_modes_to_round_function

SakiTakamachi · 2023-08-26T21:51:42Z

I personally think it looks great, but maybe an RFC is needed.

ext/standard/math.c

TimWolla · 2023-09-19T16:35:43Z

This will need to be rebased for #12220

ext/intl/tests/formatter/rounding_modes.phpt

TimWolla

Some suggestions for the implementation to unify the use of modf in a single location, instead of moving it into each branch. I did not check, but expect that to reduce the amount of assembly generated for this function.

ext/intl/tests/formatter/rounding_modes.phpt

ext/standard/math.c

ext/intl/formatter/formatter.stub.php

bukka · 2023-10-12T19:50:18Z

NEWS

@@ -25,6 +27,8 @@ Standard:
  . Partly fix GH-12143 (Incorrect round() result for 0.49999999999999994).
    (timwolla)
  . Fix GH-12252 (round(): Validate the rounding mode). (timwolla)
+  . Added 4 new rounding modes to the round(). (Jorg Sowa)


You should not be updating NEWS - this is updated by person merging the PR together with UPGRADING.

I was told in my other PRs to update UPGRADING file. So I see there are different opinions on this and I'm not sure now which should I follow.

If I update the UPGRADING file in the same PR isn't it easier for the person merging PR? This saves time for them.

Not sure who told you that but it's usually done by commiter because those files (especially NEWS) might change often so it might lead to conflicts. It's not a big issue for master only change and UPGRADING as they don't change that often but it's still usually done by commiter at the time of the commit.

It is easier to have the committer of the PR to update UPGRADING then push this task on the person merging the PR.

Especially as UPGRADING is only ever touched in master.

But NEWS, AFAIK, is only ever used to inform about bug fixes in patch releases.

I'm confused what should be included in NEWS and UPGRADING. Do you think it would be useful to add little description to the header of those files?

Apparently, and I've only learned this only recently, you should update both UPGRADING and NEWS.

The only case when UPGRADING is not updated is for bug fixes in release branches.

bukka · 2023-10-12T19:51:32Z

ext/intl/formatter/formatter.stub.php

+    /**
+     * @var int
+     * @cvalue UNUM_ROUND_DOWN
+     */
+    public const ROUND_TOWARD_ZERO = UNKNOWN;
+    /**
+     * @var int
+     * @cvalue UNUM_ROUND_UP
+     */
+    public const ROUND_AWAY_FROM_ZERO = UNKNOWN;


I think this should be done in a separate follow up PR

I have included this change in the same RFC. Do you think I should create separate PR for this, or it's fine to keep it in this?

I left this in the same PR as it was accepted in the same RFC.

ext/standard/math.c

jorgsowa · 2023-12-19T00:20:53Z

Thank you for the comments. I have addressed them all. As the RFC has been approved, I'm kindly asking for the final review of the code.

TimWolla

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!

ext/standard/tests/math/round_modes.phpt

Girgias

I'll let @TimWolla merge this.

TimWolla · 2023-12-21T12:29:38Z

@jorgsowa Now merged after re-adding some original test cases (makes the diff easier to check that there are only additions and no changes), adding UPGRADING notes and slightly rephrasing NEWS.

Thank you for your patience! Don't forget to update the RFC page to the “Implemented” status.

see 94ddc74 see php#12056

jorgsowa requested a review from bukka as a code owner August 26, 2023 20:30

github-actions bot added the Extension: standard label Aug 26, 2023

TimWolla reviewed Sep 3, 2023

View reviewed changes

ext/standard/math.c Outdated Show resolved Hide resolved

ext/standard/math.c Outdated Show resolved Hide resolved

jorgsowa force-pushed the add-new-rounding-modes-to-round branch from 581cc0e to 3116fd0 Compare September 4, 2023 21:43

TimWolla added the RFC label Sep 6, 2023

TimWolla mentioned this pull request Sep 6, 2023

Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5 #12143

Closed

jorgsowa force-pushed the add-new-rounding-modes-to-round branch from 2e6c3e0 to 9906413 Compare October 3, 2023 23:02

jorgsowa requested a review from kocsismate as a code owner October 4, 2023 21:54

github-actions bot added the Extension: intl label Oct 4, 2023

Girgias reviewed Oct 4, 2023

View reviewed changes

ext/intl/tests/formatter/rounding_modes.phpt Outdated Show resolved Hide resolved

TimWolla reviewed Oct 5, 2023

View reviewed changes

TimWolla reviewed Oct 6, 2023

View reviewed changes

ext/standard/math.c Outdated Show resolved Hide resolved

kocsismate reviewed Oct 6, 2023

View reviewed changes

ext/intl/formatter/formatter.stub.php Outdated Show resolved Hide resolved

bukka reviewed Oct 12, 2023

View reviewed changes

jorgsowa force-pushed the add-new-rounding-modes-to-round branch from 9c1e650 to e364387 Compare November 14, 2023 22:45

jorgsowa changed the title ~~Add 4 new rounding modes to round() function~~ [RFC] Add 4 new rounding modes to round() function Nov 15, 2023

TimWolla reviewed Nov 15, 2023

View reviewed changes

ext/standard/math.c Outdated Show resolved Hide resolved

TimWolla reviewed Nov 18, 2023

View reviewed changes

ext/standard/math.c Outdated Show resolved Hide resolved

ext/standard/math.c Outdated Show resolved Hide resolved

ext/standard/math.c Outdated Show resolved Hide resolved

ext/standard/math.c Outdated Show resolved Hide resolved

jorgsowa marked this pull request as draft December 3, 2023 22:27

jorgsowa added 9 commits December 18, 2023 22:59

Add 4 new rounding modes to round() function

5b89dd2

Added tests for consistency with ceil() and floor() functions

d0d76a0

Extend test cases for round modes

2c853a8

Fix returning rounded number for switch

087ebba

Clear the code after rebase

5be2200

Add aliases and deprecate constants ROUND_DOWN and ROUND_UP

40b9369

Added information about constants from Intl extension to NEWS

962f752

Undeprecate the constants

823025d

Refactored rounding function

6790846

jorgsowa added 8 commits December 18, 2023 23:01

Generate stubs

8844699

use fabs method for specific rounding modes

f4fa18c

Revert logic of fabs in rounding function

8b774e7

Revert formatting of the math.c

fd1ee38

Removed indentation of switch branches

76ab816

Removed unnecessary formatting changes

c085eee

Updated stub for basic_functions

b8e3933

Merged if conditions

7acc226

jorgsowa force-pushed the add-new-rounding-modes-to-round branch from a03abbf to 7acc226 Compare December 19, 2023 00:02

jorgsowa added 2 commits December 19, 2023 01:04

Used if condition for PHP_ROUND_AWAY_FROM_ZERO mode

2a53d44

Replaced missing colon

4b919d8

jorgsowa marked this pull request as ready for review December 19, 2023 00:20

jorgsowa requested a review from devnexen as a code owner December 19, 2023 00:20

jorgsowa requested a review from TimWolla December 19, 2023 00:21

[ci skip] Formatting

17eda27

TimWolla changed the title ~~[RFC] Add 4 new rounding modes to round() function~~ RFC: Add 4 new rounding modes to round() function Dec 19, 2023

TimWolla approved these changes Dec 19, 2023

View reviewed changes

ext/standard/tests/math/round_modes.phpt Outdated Show resolved Hide resolved

[skip ci] Renamed title of test round_modes_ceiling_and_floor.phpt

db8ffaa

Girgias reviewed Dec 20, 2023

View reviewed changes

jorgsowa and others added 4 commits December 20, 2023 20:15

Added tests of round() for zeroes

f2484f6

Removed zeroes from round() standard tests

be9e7ec

Re-add 7/-7 to round_modes.phpt

7f8b0a3

UPGRADING / NEWS

fd82487

TimWolla merged commit 94ddc74 into php:master Dec 21, 2023

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jan 12, 2024

UPGRADING: Add new PHP_ROUND_* to global constants section

e31bf3a

see 94ddc74 see php#12056

jorgsowa deleted the add-new-rounding-modes-to-round branch February 8, 2024 22:12

llaville mentioned this pull request Oct 15, 2024

[8.4.0RC2] Don't see new rounding modes in standard extension #16446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Add 4 new rounding modes to round() function #12056

RFC: Add 4 new rounding modes to round() function #12056

jorgsowa commented Aug 26, 2023 •

edited

Loading

SakiTakamachi commented Aug 26, 2023

TimWolla commented Sep 19, 2023

TimWolla left a comment

bukka Oct 12, 2023

jorgsowa Oct 14, 2023

bukka Oct 14, 2023

Girgias Oct 14, 2023 •

edited

Loading

jorgsowa Dec 18, 2023

Girgias Dec 20, 2023

bukka Oct 12, 2023

jorgsowa Nov 15, 2023

jorgsowa Dec 19, 2023

jorgsowa commented Dec 19, 2023

TimWolla left a comment

Girgias left a comment

TimWolla commented Dec 21, 2023

RFC: Add 4 new rounding modes to round() function #12056

RFC: Add 4 new rounding modes to round() function #12056

Conversation

jorgsowa commented Aug 26, 2023 • edited Loading

SakiTakamachi commented Aug 26, 2023

TimWolla commented Sep 19, 2023

TimWolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girgias Oct 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgsowa commented Dec 19, 2023

TimWolla left a comment

Choose a reason for hiding this comment

Girgias left a comment

Choose a reason for hiding this comment

TimWolla commented Dec 21, 2023

jorgsowa commented Aug 26, 2023 •

edited

Loading

Girgias Oct 14, 2023 •

edited

Loading