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

Merged
TimWolla merged 25 commits into
php:masterfrom
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged

RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom
jorgsowa:add-new-rounding-modes-to-round

Conversation

@jorgsowa

@jorgsowajorgsowa commented Aug 26, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@jorgsowa
jorgsowa requested a review from bukka as a code ownerAugust 26, 2023 20:30
@SakiTakamachi

SakiTakamachi commented Aug 26, 2023

Copy link
Copy Markdown
Member

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

TimWolla
TimWolla reviewed Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
@jorgsowa
jorgsowaforce-pushed the add-new-rounding-modes-to-round branch from 581cc0e to 3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla added the RFC label Sep 6, 2023
@TimWollaTimWolla mentioned this pull request Sep 6, 2023
Closed
@TimWolla

TimWolla commented Sep 19, 2023

Copy link
Copy Markdown
Member

This will need to be rebased for #12220

@jorgsowa
jorgsowaforce-pushed the add-new-rounding-modes-to-round branch from 2e6c3e0 to 9906413CompareOctober 3, 2023 23:02
Girgias
Girgias reviewed Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt Outdated
TimWolla
TimWolla reviewed Oct 5, 2023
View reviewed changes

@TimWollaTimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threadext/intl/tests/formatter/rounding_modes.phpt Outdated
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
TimWolla
TimWolla reviewed Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c Outdated
kocsismate
kocsismate reviewed Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php Outdated
bukka
bukka reviewed Oct 12, 2023
View reviewed changes
Comment threadNEWS Outdated

@bukkabukkaOct 12, 2023

Copy link
Copy Markdown
Member

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.

@jorgsowajorgsowaOct 14, 2023

Copy link
Copy Markdown
ContributorAuthor

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.

@bukkabukkaOct 14, 2023

Copy link
Copy Markdown
Member

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.

@GirgiasGirgiasOct 14, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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.

@jorgsowajorgsowaDec 18, 2023

Copy link
Copy Markdown
ContributorAuthor

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?

@GirgiasGirgiasDec 20, 2023

Copy link
Copy Markdown
Member

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.

Comment threadext/intl/formatter/formatter.stub.php Outdated
Comment on lines 95 to 104

@bukkabukkaOct 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@jorgsowajorgsowaNov 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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?

@jorgsowajorgsowaDec 19, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

@jorgsowa
jorgsowaforce-pushed the add-new-rounding-modes-to-round branch from 9c1e650 to e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa changed the title Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla reviewed Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c Outdated
TimWolla
TimWolla reviewed Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
Comment threadext/standard/math.c Outdated
@jorgsowa
jorgsowa marked this pull request as draft December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed the add-new-rounding-modes-to-round branch from a03abbf to 7acc226CompareDecember 19, 2023 00:02
@jorgsowa

jorgsowa commented Dec 19, 2023

Copy link
Copy Markdown
ContributorAuthor

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.

@jorgsowa
jorgsowa marked this pull request as ready for review December 19, 2023 00:20
@TimWollaTimWolla changed the title [RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla approved these changes Dec 19, 2023
View reviewed changes

@TimWollaTimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment threadext/standard/tests/math/round_modes.phpt Outdated
Girgias
Girgias reviewed Dec 20, 2023
View reviewed changes

@GirgiasGirgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.

@TimWolla
TimWolla merged commit 94ddc74 into php:masterDec 21, 2023
@TimWolla

TimWolla commented Dec 21, 2023

Copy link
Copy Markdown
Member

@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.

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa deleted the add-new-rounding-modes-to-round branch February 8, 2024 22:12
@llavillellaville mentioned this pull request Oct 15, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees

No one assigned

Labels

Extension: intlExtension: standardRFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
RFC: Add 4 new rounding modes to round() function - #12056
Merged
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Dec 21, 2023
Merged
RFC: Add 4 new rounding modes to round() function#12056
TimWolla merged 25 commits into
php:masterfrom 
jorgsowa:add-new-rounding-modes-to-round
Conversation
@jorgsowa
@jorgsowajorgsowa
commented
Aug 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@jorgsowa
jorgsowa
requested a review
from bukka
as a code ownerAugust 26, 2023 20:30
@SakiTakamachi
SakiTakamachi
commented
Aug 26, 2023

Copy link


Copy Markdown

Member
I personally think it looks great, but maybe an RFC is needed.
TimWolla
TimWolla
reviewed
Sep 3, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
581cc0e to
3116fd0CompareSeptember 4, 2023 21:43
@TimWollaTimWolla
added
the 
RFC

label
Sep 6, 2023
@TimWollaTimWolla
mentioned this pull request
Sep 6, 2023

Closed

@TimWolla
TimWolla
commented
Sep 19, 2023

Copy link


Copy Markdown

Member
This will need to be rebased for #12220
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
2e6c3e0 to
9906413CompareOctober 3, 2023 23:02
Girgias
Girgias
reviewed
Oct 4, 2023
View reviewed changes
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

TimWolla
TimWolla
reviewed
Oct 5, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadext/intl/tests/formatter/rounding_modes.phpt
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

kocsismate
kocsismate
reviewed
Oct 6, 2023
View reviewed changes
Comment threadext/intl/formatter/formatter.stub.php
Outdated

bukka
bukka
reviewed
Oct 12, 2023
View reviewed changes
Comment threadNEWS
Outdated

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaOct 14, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@bukkabukkaOct 14, 2023

Copy link


Copy Markdown

Member
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.
@GirgiasGirgiasOct 14, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@jorgsowajorgsowaDec 18, 2023

Copy link


Copy Markdown

ContributorAuthor
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?
@GirgiasGirgiasDec 20, 2023

Copy link


Copy Markdown

Member
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.
Comment threadext/intl/formatter/formatter.stub.php
Outdated


Comment on lines

95
 to 
104

@bukkabukkaOct 12, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should be done in a separate follow up PR
@jorgsowajorgsowaNov 15, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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?
@jorgsowajorgsowaDec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left this in the same PR as it was accepted in the same RFC.
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
9c1e650 to
e364387CompareNovember 14, 2023 22:45
@jorgsowajorgsowa
changed the title
Add 4 new rounding modes to round() function[RFC] Add 4 new rounding modes to round() functionNov 15, 2023
TimWolla
TimWolla
reviewed
Nov 15, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

TimWolla
TimWolla
reviewed
Nov 18, 2023
View reviewed changes
Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

Comment threadext/standard/math.c
Outdated

@jorgsowa
jorgsowa
marked this pull request as draft
December 3, 2023 22:27
@jorgsowa
jorgsowaforce-pushed
the
add-new-rounding-modes-to-round
branch
from
a03abbf to
7acc226CompareDecember 19, 2023 00:02
@jorgsowa
jorgsowa
commented
Dec 19, 2023

Copy link


Copy Markdown

ContributorAuthor
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.
@jorgsowa
jorgsowa
marked this pull request as ready for review
December 19, 2023 00:20
@TimWollaTimWolla
changed the title
[RFC] Add 4 new rounding modes to round() functionRFC: Add 4 new rounding modes to round() functionDec 19, 2023
TimWolla
TimWolla
approved these changes
Dec 19, 2023
View reviewed changes
@TimWollaTimWolla
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Implementation LGTM now. I'll likely merge at the end of the week, unless someone complains or beats me to it. Thank you!
Comment threadext/standard/tests/math/round_modes.phpt
Outdated

Girgias
Girgias
reviewed
Dec 20, 2023
View reviewed changes
@GirgiasGirgias
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll let @TimWolla merge this.
@TimWolla
TimWolla
merged commit 94ddc74
into
php:masterDec 21, 2023
@TimWolla
TimWolla
commented
Dec 21, 2023

Copy link


Copy Markdown

Member
@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.
TimWolla added a commit
to TimWolla/php-src
that referenced
this pull request
Jan 12, 2024
@TimWolla
UPGRADING: Add new PHP_ROUND_* to global constants section
e31bf3a
see 94ddc74
see php#12056
@jorgsowa
jorgsowa
deleted the

add-new-rounding-modes-to-round

branch
February 8, 2024 22:12
@llavillellaville
mentioned this pull request
Oct 15, 2024

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@bukkabukkabukka left review comments
@TimWollaTimWollaTimWolla approved these changes
@kocsismatekocsismatekocsismate left review comments
@GirgiasGirgiasGirgias left review comments
@devnexendevnexenAwaiting requested review from devnexendevnexen is a code owner

Assignees


No one assigned


Labels

Extension: intlExtension: standardRFC

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jorgsowa@SakiTakamachi@TimWolla@bukka@kocsismate@Girgias