-
Notifications
You must be signed in to change notification settings - Fork 7.9k
RFC: Add 4 new rounding modes to round() function #12056
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.
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
Merged
TimWolla
merged 25 commits into
php:master
from
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
5b89dd2
Add 4 new rounding modes to round() function
jorgsowa d0d76a0
Added tests for consistency with ceil() and floor() functions
jorgsowa 2c853a8
Extend test cases for round modes
jorgsowa 087ebba
Fix returning rounded number for switch
jorgsowa 5be2200
Clear the code after rebase
jorgsowa 40b9369
Add aliases and deprecate constants ROUND_DOWN and ROUND_UP
jorgsowa 962f752
Added information about constants from Intl extension to NEWS
jorgsowa 823025d
Undeprecate the constants
jorgsowa 6790846
Refactored rounding function
jorgsowa 8844699
Generate stubs
jorgsowa f4fa18c
use fabs method for specific rounding modes
jorgsowa 8b774e7
Revert logic of fabs in rounding function
jorgsowa fd1ee38
Revert formatting of the math.c
jorgsowa 76ab816
Removed indentation of switch branches
jorgsowa c085eee
Removed unnecessary formatting changes
jorgsowa b8e3933
Updated stub for basic_functions
jorgsowa 7acc226
Merged if conditions
jorgsowa 2a53d44
Used if condition for PHP_ROUND_AWAY_FROM_ZERO mode
jorgsowa 4b919d8
Replaced missing colon
jorgsowa 17eda27
[ci skip] Formatting
TimWolla db8ffaa
[skip ci] Renamed title of test round_modes_ceiling_and_floor.phpt
jorgsowa f2484f6
Added tests of round() for zeroes
jorgsowa be9e7ec
Removed zeroes from round() standard tests
jorgsowa 7f8b0a3
Re-add 7/-7 to round_modes.phpt
TimWolla fd82487
UPGRADING / NEWS
TimWolla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
--TEST-- | ||
NumberFormatter: rounding modes | ||
--EXTENSIONS-- | ||
intl | ||
--FILE-- | ||
<?php | ||
ini_set("intl.error_level", E_WARNING); | ||
|
||
var_dump(NumberFormatter::ROUND_CEILING); | ||
var_dump(NumberFormatter::ROUND_FLOOR); | ||
var_dump(NumberFormatter::ROUND_DOWN); | ||
var_dump(NumberFormatter::ROUND_TOWARD_ZERO); | ||
var_dump(NumberFormatter::ROUND_UP); | ||
var_dump(NumberFormatter::ROUND_AWAY_FROM_ZERO); | ||
var_dump(NumberFormatter::ROUND_HALFEVEN); | ||
var_dump(NumberFormatter::ROUND_HALFDOWN); | ||
var_dump(NumberFormatter::ROUND_HALFUP); | ||
|
||
var_dump(NumberFormatter::ROUND_DOWN === NumberFormatter::ROUND_TOWARD_ZERO); | ||
var_dump(NumberFormatter::ROUND_UP === NumberFormatter::ROUND_AWAY_FROM_ZERO); | ||
?> | ||
--EXPECTF-- | ||
int(0) | ||
int(1) | ||
int(2) | ||
int(2) | ||
int(3) | ||
int(3) | ||
int(4) | ||
int(5) | ||
int(6) | ||
bool(true) | ||
bool(true) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not be updating NEWS - this is updated by person merging the PR together with UPGRADING.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.