Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123020
Message-ID: <959ccc6e-1d5c-48c0-9357-80e38becd1c5@bastelstu.be>
Date: Sun, 7 Apr 2024 19:09:34 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PHP-DEV] [RFC] [Discussion] Support object type in BCMath
To: Saki Takamachi <saki@sakiot.com>
Cc: internals@lists.php.net
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>
Content-Language: en-US
In-Reply-To: <B6391516-7680-4B7C-BFBA-D5DEE274F030@sakiot.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

On 4/6/24 17:07, Saki Takamachi wrote:
> To you all:
> The discussions thus far have been reflected in the RFC. I would be very happy if you could check it out. (Thanks Tim, the <PHP> tag is very easy to read.)
> https://wiki.php.net/rfc/support_object_type_in_bcmath

Thank you for the updates. Some remarks:

- The namespace is inconsistently referred to as "BCMath" and "BcMath", 
please check the entire RFC to unify the casing.

- The full “stub” should probably include an explicit "implements 
Stringable" for clarity.

- 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.

- The property should probably be named "$roundingMode" instead of 
"$roundMode".

- 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.

- In the stub there's a typo: "puclic", should be "public".

- 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'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'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.

- For the RFC Impact on SAPIs, it should simply be "None", because SAPIs 
do not need to do anything special.

- For the Backwards Incompatible Changes, it should list that the 
BcMath\Number class will no longer be available to userland.

Best regards
Tim Düsterhus