Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122787
Date: Thu, 28 Mar 2024 12:00:46 +0000 (GMT)
To: Saki Takamachi <saki@sakiot.com>
cc: php internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Support object type in BCMath
In-Reply-To: <68CF373E-6ABF-4471-8992-813B4BA1B508@sakiot.com>
Message-ID: <62e27e46-f911-5a71-6527-8bba21bf0964@php.net>
References: <4F094EDA-5058-407D-AF39-06FD934FDE1F@sakiot.com> <bbcff629-4dd6-bc50-ac9f-641849dc218a@php.net> <68CF373E-6ABF-4471-8992-813B4BA1B508@sakiot.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-452057772-1711627249=:14553"
From: derick@php.net (Derick Rethans)

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-452057772-1711627249=:14553
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Wed, 27 Mar 2024, Saki Takamachi wrote:

> > - You've picked as class name "BcNum". Following
> >  our naming guidelines, that probably should be \BCMath\Num (or=20
> >  \BC\Num, but that is less descriptive):=20
> >  https://github.com/php/policies/blob/main/coding-standards-and-naming.=
rst#namespaces-in-extensions
> >=20
> >  The reason it *should* have "BC" is that it comes from "Basic=20
> >  Calculator" (https://www.php.net/manual/en/book.bc.php#118203)
>=20
>=20
> I re-read the namespace RFC again. I also re-read the RFC regarding=20
> class naming conventions.=20
> https://wiki.php.net/rfc/namespaces_in_bundled_extensions=20
> https://wiki.php.net/rfc/class-naming
>=20
> There's no need for the namespace to follow class naming conventions,=20
> but the acronym doesn't seem to need to be pascal-case anyway (I=20
> remembered it incorrectly). However, the RFC states that the=20
> extension's namespace must match the extension name, so it seems=20
> correct in this case for the namespace to be `BCMath`.
>=20
> And indeed, looking at the example in the namespace RFC,=20
> `BCMath\Number` might be appropriate in this case (I think I was=20
> sleepy yesterday).
>=20
> I changed `BcNum` to `BCMath\Number` in my RFC.

That works fine as well. Especially as most people would add:

use BCMath\Number as Number;

and then just use "Number" everywhere.

> > - Should ->value rather be ->toString() ? ->value alone doesn't=20
> > really
> >  say much. I'm on the fence here though, as there is already=20
> >  (internally) a ->__toString() method to make the (string) cast=20
> >  work.
>=20
> What is the main difference between getting a read-only property with=20
> `->value` and getting the value using a method?

Feeling :-) Do we have precedence with other extension's objects=20
perhaps already?

> > - Would it make sense to have "floor" and "ceil" to also have a scale,=
=20
> >  or precision? Or would developers instead have to use "round" in=20
> >  that case?
>=20
> > - Which rounding modes are supported with "round", the same ones as=20
> > the
> >  normal round() function?
>=20
> `bcfloor` and `bcceil` originally have no scale specification. This is=20
> because the result is always a string representing an integer value.=20
> And since the supported round-mode is the same as standard-round,=20
> `ROUND_FLOOR` and `ROUND_CEILING` are also supported. Therefore, if=20
> want to obtain floor or ceil behavior with a specified scale, I=20
> recommend specifying the mode as round.

OK. That makes sense.

> > - In this example, what would $result->scale show? (Perhaps add that to=
=20
> >  the example?):
> >=20
> > <?php
> > $num =3D new BcNum('1.23', 2);
> > $result =3D $num + '1.23456';
> > $result->value; // '2.46456'
> > $result->scale; // ??
>=20
> In this case, `$result->scale` will be `'5'`. I added this to the RFC.

Great.=20

> > - Exceptions
> >=20
> >  The RFC does not mention which exceptions can be thrown. Is it just=20
> >  the one? It might be beneficial to *do* have a new exception=20
> >  hierarchy.
>=20
>=20
> As far as I know right now, following exceptions can be thrown:
>=20
> - Value error when a string that is invalid as a number is used in a=20
>   constructor, calculation method, or operation
> - Divide by 0 error (include Modulo by zero)
>=20
> I was thinking that it would be a bad idea to increase the number of=20
> classes without thinking, and was planning to use general exceptions,=20
> but would it be better to use dedicated exceptions?

It's what we did for the Date extension, and the Random extension, but=20
in this case, it's probably not needed as you say.

> By the way, generally when implementing such exceptions in userland,=20
> value errors and divide-by-zero errors are probably defined as=20
> separate classes, but should they be separated in this case?

For that, yes. ValueErrors should be distinct from DivideByZeroError =E2=80=
=94 I=20
think we do have both of those already:

php -r 'echo 8/0;'

Fatal error: Uncaught DivisionByZeroError: Division by zero in Command=20
line code on line 1

From=20the docs for ValueError:

"A ValueError is thrown when the type of an argument is correct but the=20
value of it is incorrect."

From=20the docs for DivisionByZeroError:

" DivisionByZeroError is thrown when an attempt is made to divide a=20
number by zero. "

Subclassing these for BCMath objects seems unnecessary therefore.

cheers,
Derick

--=20
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/suppo=
rt

mastodon: @derickr@phpc.social @xdebug@phpc.social
--8323329-452057772-1711627249=:14553--