Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123027
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6.1.1\))
Subject: Re: [PHP-DEV] [RFC] [Discussion] Support object type in BCMath
In-Reply-To: <959ccc6e-1d5c-48c0-9357-80e38becd1c5@bastelstu.be>
Date: Mon, 8 Apr 2024 09:08:56 +0900
Cc: internals@lists.php.net
Content-Transfer-Encoding: quoted-printable
Message-ID: <B7F2D363-BDCC-4AEA-A1AE-782C14DAD37A@sakiot.com>
References: <D1E3EBAD-A991-4591-B7DE-CF414593E64B@sakiot.com>
 <FB449C0E-1D83-476F-889D-19A6ABDBD38E@sakiot.com>
 <FF0FFBCE-7F03-44A2-B7AE-D3F958135ACB@sakiot.com>
 <b232c9f1-aad9-4ef3-8f23-d0cb2deb0baf@rwec.co.uk>
 <B6391516-7680-4B7C-BFBA-D5DEE274F030@sakiot.com>
 <959ccc6e-1d5c-48c0-9357-80e38becd1c5@bastelstu.be>
To: =?utf-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
From: saki@sakiot.com (Saki Takamachi)

Hi Tim,

> - The namespace is inconsistently referred to as "BCMath" and =
"BcMath", please check the entire RFC to unify the casing.

Thanks, I fixed it.


> - The full =E2=80=9Cstub=E2=80=9D should probably include an explicit =
"implements Stringable" for clarity.

Agree. I describe it explicitly during implementation.


> - I don't want to expand the scope of your RFC further than necessary, =
but for the rounding mode, I wonder if we should first add the =
RoundingMode enum that has been suggested during the discussion of the =
"Add 4 new rounding modes to round() function" RFC. It would make the =
type for the `Number::$roundMode` property much clearer than the current =
`int`. I expect the addition of such an enum to be a pretty simple RFC =
that would not need much discussion. I'd be happy to co-author it.

Oh, that's a good idea. Makes sense. I think it would be simpler to =
prepare an RFC separate from this RFC, so I'm going to create one right =
away. Once you have a certain amount of content, I would be happy if you =
could check it out and make corrections if necessary.


> - The property should probably be named "$roundingMode" instead of =
"$roundMode".

Thank you, I'm not a native speaker, so pointing out English expressions =
is very helpful. I fixed it.


> - I'm not sure I like the differing default rounding modes for =
implicit rounding (i.e. the $roundMode property) and explicit rounding =
(i.e. the round() method). That sounds like it would cause unnecessary =
confusion.

I missed the point. All defaults have been changed to HALF_UP.


> - In the stub there's a typo: "puclic", should be "public".

Oops, thanks!


> - For `format()`, I'm not sure whether we should actually add the =
function. While we have number_format() for regular numbers with the =
same signature, it fails to account for the different language and =
cultures in the world. Specifically `number_format()` cannot correctly =
format numbers for en_IN (i.e. English in India). In India numbers are =
not separated every three digits, but rather the three right-most digits =
and then every two digits. Here's in example: 1,23,45,67,890. I believe =
formatting should be best left for ext/intl.

I'm not very familiar with ext/intl, but is there a way to format a =
string type number without converting it to a float?


> - I'd probably not add the 'with()' method, because it would be =
redundant with https://wiki.php.net/rfc/clone_with and it can already be =
specified by a combination of withMaxExpansionScale + withRoundMode.

I see, I wasn't aware of that argument. Removed `with()`.


> - I'm not sure if the priority for the rounding modes is sound. My gut =
feeling is that operations on numbers with different rounding modes =
should be disallowed or made explicit during the operation (much like =
the scale for a division), but I'm not an expert in designing numeric =
APIs, so I might be wrong here.

As mentioned in the RFC, the problem here is commutative calculations =
(add, sub, mul). This means that even if specify `$roundingMode` during =
these calculations, `$roundingMode` will not be used in these =
calculations. Calculations that use `$roundingMode` are divs, etc. If =
allow `$roundingMode` to be specified with add, intuitively it feels =
like the result of add will be rounded.

Also, it is not possible to specify `$roundMode` in calculations using =
operators.

However, if the user calculates two numbers with different rounding =
modes, and it is intentional rather than a mistake, I can't imagine what =
kind of result the user would want to get. Therefore, it may be better =
to make this an error.

Is it appropriate to throw a value error in that case?


> - For the RFC Impact on SAPIs, it should simply be "None", because =
SAPIs do not need to do anything special.

I fixed it.


> - For the Backwards Incompatible Changes, it should list that the =
BcMath\Number class will no longer be available to userland.

I added that.


Thanks!

Saki