Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123273
Date: Tue, 07 May 2024 15:23:49 +0000
To: Saki Takamachi <saki@sakiot.com>
Cc: php internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] [vote] Support object type in BCMath
Message-ID: <pjiOa-Rruk4s7NZVa_1doLG_UYGly0A_J2VPfjU-AhddTI_6XQ6m1Ih5oSMYOaEZp73i4MvfCmcg0N2P1IWcthfzSXgEifhDxL29eCQdxC0=@gpb.moe>
In-Reply-To: <9BD04F6A-95FA-4C04-AE9C-B9ACB92A9D58@sakiot.com>
References: <58283467-CABC-4868-A691-A52228C5296F@sakiot.com> <6Kx-yDO_RoWi8qV0NDhpziE5VX-cDF3g5RCEUjqh5iOnM2x2pWtYPz0j3bNUz-BdkkIVXXG38tu8fP9FuAiQYl2oDcTYAUivNn63B10Vw9I=@gpb.moe> <9BD04F6A-95FA-4C04-AE9C-B9ACB92A9D58@sakiot.com>
Feedback-ID: 96993444:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: internals@gpb.moe ("Gina P. Banyard")

On Tuesday, 7 May 2024 at 15:46, Saki Takamachi <saki@sakiot.com> wrote:

> Hi Gina,
>=20
> > I voted in favour, as Number being its own type should improve DX and p=
erformance for small numbers as we can just store two 64bit integer, one fo=
r the integral and another for the fractional part.
> > However, I have some remarks and questions.
> > Apologies if these were already brought up, but I haven't gone through =
the whole 100 long email thread.
>=20
>=20
> First of all, I have to clear up your misconception.
> Number is an object that uses a structure held internally by BCMath. Ther=
efore, the data structure will be the same as the existing BCMath. In other=
 words, supporting object types does not dramatically improve performance o=
r introduce new computational logic.
>=20
> Even if the data structure is different from the existing one, just two 6=
4-bit values are probably not enough for BCMath.
>=20
> This may be off-topic, as you may know, BCMath is, in my opinion, a type =
of what is called BCD (Binary-coded decimal). If we expect to improve perfo=
rmance while maintaining these characteristics, we may want to consider a d=
ata type called DPD (densely packed decimal). (I'm doing some refactoring r=
ight now, but the changes to the data structure will be quite extensive, so=
 I haven't thought much about it yet.)

If we are not going to use the newfound opaque type to introduce performanc=
e optimization for the common cases of small numbers that fit into 64-bit i=
ntegers, then the value of adding a dedicated object is vastly diminished.
Python is *literally* doing this sort of optimizations to improve the speed=
 of their arbitrary precision integer type for the common use case of "smal=
l integers".

In general, if it cannot fit in a native type then it should continue to us=
e a string, but I would expect the *vast* majority of cases to fit in a 64-=
bit integer.


>=20
> > - The comparison method should either be called "cmp", or "compare" to =
align with other extensions, the BC Math function being called bc_comp is w=
eird.
> > - The "powmod" method should probably be called "powMod"
>=20
>=20
> I named to match existing function names [1]. Admittedly, these names may=
 seem a little strange. However, I didn't change this because it would crea=
te a new debate about consistency with existing BC function naming and comp=
licate the RFC.

I am not sure how using better names complicates an RFC.

>=20
> > - I don't understand why we are exposing lt, lte, gt, and gte what is t=
he rationale here? Also, the naming is kinda terrible IMHO.
>=20
>=20
> Does that mean it's unnecessary because it can be compared by operator ov=
erload? Or it's unnecessary because `comp()` exists?
>=20
> Regarding naming: These names are also used in Carbon, a php date and tim=
e library (these are shorthand name methods) [2]. It is possible to name so=
mething like `greaterThanOrEqualTo`, but I personally felt it was a little =
too long, so I shortened it.
>=20
> Names like these are also used in Laravel's validation rules. Therefore, =
for PHP users, at least I don't think it's something that makes no sense wh=
en see it for the first time.


In general, I am not sure why there is a need in the first place to expose =
any of the overloaded operators as methods.
This includes the add/sub/div/mult/mod methods, but I can be convinced that=
 this makes some sense.

However, I see no reason for exposing lt/gt/gte/lte.
I am also not really a fan of the compare and equal methods to be instance =
methods compared to static methods.
Adding those methods might interfere with any potential RFC that adds an Eq=
uatable/Ordable interface to allow userland classes to overload the compari=
son operators.

I was planning on tackling our rather broken comparison semantics [1] this =
year, but got distracted by the broken container/offset semantics instead. =
[2]
As, IMHO, we need to fix the internal object handlers and semantics first b=
efore giving this power to userland.
And I feel strongly that using instance methods for overload is the "wrong"=
 way of doing it.
Moreover, if we do provide the option for userland to overload they should,=
 again IMHO, *just* be able to overload =3D=3D and <=3D>, not all the indiv=
idual comparison operators.
Thus, providing all those methods makes no sense to me.

Moreover, taking inspiration from userland for naming methods because they =
*cannot* overload operators seems backwards.
Userland classes, if they want to provide such functionality, *must* implem=
ent a wide range of methods, however an internal class does is not required=
 to do this.
Therefore, I am not really convinced by following suit on userland nomencla=
ture.


>=20
> > - Are the existing BCMath function going to be adapted to handle the ne=
w Number object or not?
>=20
>=20
> No, that is not considered at this time.

I am struggling to see a reason for not considering this.

>=20
> > - What is the behaviour when casting a Number object to bool? Does it a=
lways cast to true like a "standard" object, or does a Number equal to 0 ca=
st to false?
>=20
>=20
> That's a good topic. I didn't include any mention of this in the RFC beca=
use there was no discussion about it. Personally, I like to return a bool v=
alue that reflects the internal numerical state, like an int.
>=20
> However, since I would like to avoid adding such specifications later whe=
n voting has already begun, I will use the same as a "standard object" for =
now. If this RFC is passed, I will additionally propose it in another RFC.

The lack of discussion about this topic is not a good excuse for not having=
 addressed this in the RFC.
This topic should have come up on its own while writing the RFC as this is =
something that you should have considered, in the same way you have conside=
red how casting to string works.
Moreover, RFC discussions are steered by the content of the RFC, and non-me=
ntioned issues requires other people to consider cases that have not been e=
xplicitly address, something that is difficult.

>=20
> > - Considering the property hook RFC has been accepted, should the $valu=
e property be a "virtual" property or not?
>=20
>=20
> I think that's probably so. But my understanding is that in this case I d=
on't think it makes any difference to the user.

ACK.


I feel this RFC has been brought to a vote too early, as such I am changing=
 my vote to no.
I am generally in favour of the concept, but there are too many details tha=
t are unanswered or didn't even get proper attention for me to feel comfort=
able with the state of the RFC in question.

Best regards,

Gina P. Banyard


[1] https://github.com/Girgias/php-rfcs/blob/master/comparison-equality-sem=
antics.md
[2] https://github.com/Girgias/php-rfcs/blob/master/container-offset-behavi=
our.md