[RFC] Add bcfloor, bcceil and bcround to BCMath #13096

SakiTakamachi · 2024-01-09T07:04:02Z

RFC implementation: https://wiki.php.net/rfc/adding_bcround_bcfloor_bcceil_to_bcmath

SakiTakamachi · 2024-01-09T16:40:10Z

There is a change in the stub, but you aren't included in the reviewers 🤔

kocsismate · 2024-01-13T09:33:47Z

There is a change in the stub, but you aren't included in the reviewers 🤔

Thanks for letting me know! I was wondering indeed for a long time, why I'm not included in many of the PRs changing stubs. After some investigation, I realized the problem and pushed d54e2f9 (details are in the description).

Regarding your PR, it looks fine to me as far as I'm qualified to tell, but I'm not a math expert... So I cannot approve it. :/ Hopefully, Gina or Niels can help review it. :)

kocsismate · 2024-01-13T09:37:29Z

ext/bcmath/bcmath.c

+	bc_init_num(&result);
+
+	if (php_str2num(&num, ZSTR_VAL(numstr)) == FAILURE) {
+		zend_argument_value_error(1, "is not well-formed");


I miss tests for the error cases (this one and a few more below)

Thanks for checking!
I added value error case to tests.

Please add a case with a nul byte in the string. The current API doesn't support them as it basically ignores anything past the nul byte.

nielsdos

I didn't look thoroughly at the tests or tested this myself, but I have some preliminary feedback.
It mostly looks correct.

ext/bcmath/bcmath.c

ext/bcmath/libbcmath/src/floor_or_ceil.c

ext/bcmath/libbcmath/src/round.c

SakiTakamachi · 2024-01-14T02:29:50Z

@nielsdos
I fixed them

Girgias

I didn't have a proper look at the test results yet

ext/bcmath/tests/bcceil.phpt

ext/bcmath/tests/bcfloor.phpt

ext/bcmath/tests/bcceil.phpt

Girgias · 2024-01-14T12:20:54Z

ext/bcmath/bcmath.c

+	bc_init_num(&result);
+
+	if (php_str2num(&num, ZSTR_VAL(numstr)) == FAILURE) {
+		zend_argument_value_error(1, "is not well-formed");


Please add a case with a nul byte in the string. The current API doesn't support them as it basically ignores anything past the nul byte.

Girgias · 2024-01-14T12:21:54Z

ext/bcmath/libbcmath/src/floor_or_ceil.c

+                Western Washington University
+                Bellingham, WA 98226-9062
+
+*************************************************************************/


This license header file looks invalid, not sure what should be used here. Be that the usual PHP License or another one.

@ramsey do you know by any chance what would be a reasonable licence to use here?

Girgias · 2024-01-14T12:37:10Z

ext/bcmath/libbcmath/src/round.c

+	*
+	* 1. When rounding to an integer part, when trying to round to a digit
+	*    larger than the num.
+	* 2. If try to round to the finest digit of num or finer than that,


What do you mean by "finest" here?

I mean "smallest digit". For example, if num is 1.23456, that digit means 6.
What is the appropriate way to express it in English?

ext/bcmath/libbcmath/src/round.c

Girgias · 2024-01-14T12:47:09Z

ext/bcmath/libbcmath/src/round.c

+	/* Loop through the remaining digits. */
+	size_t count = num->n_len + num->n_scale - rounded_len - 1;
+	nptr++;
+	while ((count > 0) && (*nptr++ == 0)) count--;


CS

Suggested change

while ((count > 0) && (*nptr++ == 0)) count--;

while ((count > 0) && (*nptr++ == 0)) {

count--;

}

Also am I missing something or why are we only checking if the digit is 0 ? This does not seem to take into account the case where we need to decide if we need to go up or down on 5?

For example, for the following code:

bcround('0.250000', 1, PHP_ROUND_HALF_UP);

Here it is checking for 0000 after 0.25. If num is 0.250001, it is not an edge case, so it judge whether it is an edge case by checking whether all the remaining values are 0.

Girgias · 2024-01-14T12:49:41Z

ext/bcmath/php_bcmath.h

@@ -19,6 +19,8 @@

 #include "libbcmath/src/bcmath.h"
 #include "zend_API.h"
+#include "php.h"


Why does this need to be included?

I tried to get rid of plenty of includes in bcmath.

I want to read and use round constants from php_math.h, but I need to resolve PHPAPI.
If you have any good ideas, it would be helpful if you could let me know.

Ahhh right it's that issue... one "dumb" solution is to redefine the constants for BCMATH and expose them and have a PHPT test to check that the values are really equal to each other.

I've been thinking about this for a while now, how if I separate out just the standard round-mode constants into a file like "php_math_round.h" and include that?

ext/bcmath/libbcmath/src/round.c

nielsdos · 2024-01-14T12:54:17Z

ext/bcmath/libbcmath/src/floor_or_ceil.c

+	/* check fractional part. */
+	size_t count = num->n_scale;
+	char *nptr = num->n_value + num->n_len;
+	while ((count > 0) && (*nptr++ == 0)) count--;


Perhaps this can become an additional utility function in bc_zero.c, but it's not too important.

SakiTakamachi · 2024-01-14T13:38:54Z

Thank you all, I will check and correct them one by one!

SakiTakamachi · 2024-03-10T02:55:07Z

@Girgias

Please add a case with a nul byte in the string. The current API doesn't support them as it basically ignores anything past the nul byte.

I think it would probably be better to add a null check to php_str2num, what do you think?

SakiTakamachi · 2024-03-10T04:47:37Z

In response to some comments, and have split the test.

2d0a52f
304cde9

Girgias

I have checked the tests for ceil and floor and they seem correct.

Haven't done rounding modes yet.

The licence is still leaving me puzzled.

ext/bcmath/tests/bcceil.phpt

ext/bcmath/libbcmath/src/round.c

SakiTakamachi · 2024-03-16T04:45:19Z

I set the test expected value to left pad and the license to php license.

nielsdos · 2024-03-16T13:05:28Z

ext/bcmath/tests/bcfloor.phpt

+--EXPECT--
+0 => 0
+                                              0.00 => 0
+                                                -0 => 0


I wonder whether the result should be -0 ? (Similarly for cases below and for bcceil)
It would be consistent with: var_dump(floor(-0.0));
Genuine question, I don't know the answer for sure.

BCMath does not currently support -0.

php-src/ext/bcmath/libbcmath/src/num2str.c

Line 45 in b06c95b

signch = num->n_sign != PLUS && !bc_is_zero_for_scale(num, MIN(num->n_scale, scale));

https://3v4l.org/HteIO

It seems worth supporting, but I'd like to keep it in a separate scope from this pull request.

Okay, I understand.

ext/bcmath/tests/bcround_early_return.phpt

nielsdos

Lgtm. Maybe wait for Gina for a final review round, as she is the code owner.
Also don't forget to add an entry to NEWS and UPGRADING.

SakiTakamachi · 2024-03-17T10:36:31Z

@nielsdos
Of course, thanks!

Co-authored-by: Gina Peter Banyard <girgias@php.net>

SakiTakamachi · 2024-04-14T13:33:58Z

BTW, Regarding the second argument of bcround, during the discussion on object support, it was pointed out that precision may have a different meaning.
The argument names of standard's round() are also the same, what do you think is the best way to do this?

Girgias · 2024-04-14T13:53:56Z

Brilliant, so PHP is just wrong: https://www.calculatorsoup.com/calculators/math/rounding-methods-calculator.php?method=round+half+down&x=-1.5&round=0&action=solve

Half up means to always do +0.5 on the boundary, and half down means to always do -0.5 on the boundary.
See: https://en.wikipedia.org/wiki/Rounding

So PHP has just been wrong for decades. But at least it is in good company in that Java, Python, Ruby, and probably others also use the wrong terminology.

I'm going to email internals explaining this situation, as I'm not sure how to "fix" this, especially in light of: https://wiki.php.net/rfc/new_rounding_modes_to_round_function

SakiTakamachi · 2024-04-14T15:30:43Z

@Girgias
Thanks for your mail.

There also seem to be missing some tests for integer precisions being negative on the boundary: e.g. 50 with a precision of -3

Since this is an early return pattern, it is included in bcround_early_return.phpt.

Girgias · 2024-04-14T21:12:06Z

@Girgias Thanks for your mail.

There also seem to be missing some tests for integer precisions being negative on the boundary: e.g. 50 with a precision of -3

Since this is an early return pattern, it is included in bcround_early_return.phpt.

I might be blind, but I don't think I saw a case where all the different half rounding modes are tested on such a boundary condition.

SakiTakamachi · 2024-04-15T00:06:21Z

@Girgias

I might be blind, but I don't think I saw a case where all the different half rounding modes are tested on such a boundary condition.

Indeed, I only tested HALF_UP, thank you. Added all modes to bcround_early_return.phpt.

Girgias

You still haven't added the tests I want, I want something like the following:

$modes = [
    'PHP_ROUND_HALF_UP',
    'PHP_ROUND_HALF_DOWN',
    'PHP_ROUND_HALF_EVEN',
    'PHP_ROUND_HALF_ODD',
    'PHP_ROUND_FLOOR',
    'PHP_ROUND_CEILING',
    'PHP_ROUND_AWAY_FROM_ZERO',
    'PHP_ROUND_TOWARD_ZERO',
];

foreach ($modes as $mode) {
    var_dump(round(50, -2, constant($mode)));
}

which, according to 3v4l.org (https://3v4l.org/8J3Mu/rfc#vgit.master) returns:

float(100)
float(0)
float(0)
float(100)
float(0)
float(100)
float(100)
float(0)

Added boundary value test case. Also removed test cases that should not be returned early from early return tests

SakiTakamachi · 2024-04-15T23:17:12Z

@Girgias

You have good eyes. Indeed, that test case failed.

I added and modified the test cases, then modified the code. As you taught me before, I am splitting commits.

Thanks!

(edit)
Ah, after looking at it again, I think I can write it a little more efficiently.

The comparison conditions for determining early returns were incorrect and have been corrected. Added processing when the rounding result is carried forward or becomes 0.

SakiTakamachi · 2024-04-16T00:11:21Z

Fix completed

Girgias

We got there in the end 🎉

Other than a very minor comment as I don't see why this works, but the tests look correct now :)

Girgias · 2024-04-29T13:36:30Z

ext/bcmath/libbcmath/src/round.c

+			}
+			break;
+
+		case PHP_ROUND_HALF_DOWN:


Can't the half down case be determined without a loop? If it does require a loop how come HALF UP doesn't need one?

For example, let's compare two cases: 5.0000 and 5.0001.

HALF_UP: Either way, it'll be a 10.
HALF_DOWN: 5.0001 will be 10, but 5.0000 will be 0.

Therefore, in the case of HALF_UP, there is no need to loop because it always carries up regardless of the loop result.

Oh wait, I was refactoring BCMath in another PR and found that trailing zeros are discarded when converting strings to bc_num structs.

So in all cases I don't need to loop, just check if there is one more digit.

github-actions bot added Category: Build System Extension: bcmath labels Jan 9, 2024

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch 2 times, most recently from e189f49 to 469b079 Compare January 9, 2024 08:22

github-actions bot added the Category: Optimizer label Jan 9, 2024

SakiTakamachi changed the title ~~RFC: Add bcfloor, bcceil and bcround to BCMath~~ [RFC] Add bcfloor, bcceil and bcround to BCMath Jan 9, 2024

SakiTakamachi marked this pull request as ready for review January 9, 2024 08:57

SakiTakamachi requested review from dstogov and Girgias as code owners January 9, 2024 08:57

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 469b079 to 17db35e Compare January 9, 2024 15:07

kocsismate requested a review from nielsdos January 13, 2024 09:33

kocsismate reviewed Jan 13, 2024

View reviewed changes

nielsdos requested changes Jan 13, 2024

View reviewed changes

ext/bcmath/bcmath.c Show resolved Hide resolved

ext/bcmath/bcmath.c Show resolved Hide resolved

ext/bcmath/libbcmath/src/floor_or_ceil.c Outdated Show resolved Hide resolved

ext/bcmath/libbcmath/src/round.c Outdated Show resolved Hide resolved

Girgias reviewed Jan 14, 2024

View reviewed changes

nielsdos requested changes Jan 14, 2024

View reviewed changes

SakiTakamachi added the RFC label Jan 18, 2024

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from e8b6c8d to 524dc20 Compare March 10, 2024 02:08

Girgias reviewed Mar 11, 2024

View reviewed changes

ext/bcmath/tests/bcceil.phpt Outdated Show resolved Hide resolved

ext/bcmath/libbcmath/src/round.c Outdated Show resolved Hide resolved

nielsdos reviewed Mar 16, 2024

View reviewed changes

nielsdos approved these changes Mar 17, 2024

View reviewed changes

SakiTakamachi mentioned this pull request Mar 17, 2024

[WIP] Prototype: Support object types in BCMath #13741

Draft

SakiTakamachi requested a review from bukka as a code owner March 30, 2024 22:56

SakiTakamachi and others added 9 commits April 14, 2024 22:30

splitted the test

ad5247f

Changed expected value to STR_PAD_LEFT

c5cbc18

Fixed the comment

c5a87bf

Co-authored-by: Gina Peter Banyard <girgias@php.net>

Changed to PHP license

caae7c7

Fixed forgetting to correct the test

71fdefe

Fixed test expectations

34415a1

Separated round mode into separate header file

94c3830

add pad

58b2657

address comment

2e2dd10

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 44f37d4 to 2e2dd10 Compare April 14, 2024 13:30

fixed config.w32

963a6b4

address comment

7c900f0

Girgias requested changes Apr 15, 2024

View reviewed changes

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 11b24b5 to 8226342 Compare April 15, 2024 15:08

Fix the test case and the test fails

d227a94

Added boundary value test case. Also removed test cases that should not be returned early from early return tests

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 8226342 to d227a94 Compare April 15, 2024 15:10

Fixed round.c

a3d08e5

The comparison conditions for determining early returns were incorrect and have been corrected. Added processing when the rounding result is carried forward or becomes 0.

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 98ea4a0 to a3d08e5 Compare April 16, 2024 00:10

SakiTakamachi requested a review from Girgias April 16, 2024 00:11

Girgias approved these changes Apr 29, 2024

View reviewed changes

When rounding to a precision finer than num, add 0 at the end

9cf63f8

SakiTakamachi force-pushed the rfc/bcmath-add-functions branch from 4388b43 to 9cf63f8 Compare April 29, 2024 17:31

Girgias merged commit 5359392 into php:master Apr 30, 2024
10 checks passed

[RFC] Add bcfloor, bcceil and bcround to BCMath #13096

[RFC] Add bcfloor, bcceil and bcround to BCMath #13096

Conversation

SakiTakamachi commented Jan 9, 2024

SakiTakamachi commented Jan 9, 2024

kocsismate commented Jan 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Jan 14, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Jan 14, 2024

SakiTakamachi commented Mar 10, 2024

SakiTakamachi commented Mar 10, 2024 • edited

Girgias left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Mar 17, 2024

SakiTakamachi commented Apr 14, 2024

Girgias commented Apr 14, 2024

SakiTakamachi commented Apr 14, 2024

Girgias commented Apr 14, 2024

SakiTakamachi commented Apr 15, 2024

Girgias left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Apr 15, 2024 • edited

SakiTakamachi commented Apr 16, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Mar 10, 2024 •

edited

SakiTakamachi commented Apr 15, 2024 •

edited