On Tuesday, 7 May 2024 at 15:46, Saki Takamachi <s...@sakiot.com> wrote:
> Hi Gina, > > > I voted in favour, as Number being its own type should improve DX and > > performance for small numbers as we can just store two 64bit integer, one > > for 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. > > > First of all, I have to clear up your misconception. > Number is an object that uses a structure held internally by BCMath. > Therefore, the data structure will be the same as the existing BCMath. In > other words, supporting object types does not dramatically improve > performance or introduce new computational logic. > > Even if the data structure is different from the existing one, just two > 64-bit values are probably not enough for BCMath. > > 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 > performance while maintaining these characteristics, we may want to consider > a data type called DPD (densely packed decimal). (I'm doing some refactoring > right 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 performance optimization for the common cases of small numbers that fit into 64-bit integers, 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 "small integers". In general, if it cannot fit in a native type then it should continue to use a string, but I would expect the *vast* majority of cases to fit in a 64-bit integer. > > > - The comparison method should either be called "cmp", or "compare" to > > align with other extensions, the BC Math function being called bc_comp is > > weird. > > - The "powmod" method should probably be called "powMod" > > > 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 create > a new debate about consistency with existing BC function naming and > complicate the RFC. I am not sure how using better names complicates an RFC. > > > - I don't understand why we are exposing lt, lte, gt, and gte what is the > > rationale here? Also, the naming is kinda terrible IMHO. > > > Does that mean it's unnecessary because it can be compared by operator > overload? Or it's unnecessary because `comp()` exists? > > Regarding naming: These names are also used in Carbon, a php date and time > library (these are shorthand name methods) [2]. It is possible to name > something like `greaterThanOrEqualTo`, but I personally felt it was a little > too long, so I shortened it. > > 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 when 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 Equatable/Ordable interface to allow userland classes to overload the comparison 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 before 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 == and <=>, not all the individual 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* implement 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 nomenclature. > > > - Are the existing BCMath function going to be adapted to handle the new > > Number object or not? > > > No, that is not considered at this time. I am struggling to see a reason for not considering this. > > > - What is the behaviour when casting a Number object to bool? Does it > > always cast to true like a "standard" object, or does a Number equal to 0 > > cast to false? > > > That's a good topic. I didn't include any mention of this in the RFC because > there was no discussion about it. Personally, I like to return a bool value > that reflects the internal numerical state, like an int. > > However, since I would like to avoid adding such specifications later when > 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 considered how casting to string works. Moreover, RFC discussions are steered by the content of the RFC, and non-mentioned issues requires other people to consider cases that have not been explicitly address, something that is difficult. > > > - Considering the property hook RFC has been accepted, should the $value > > property be a "virtual" property or not? > > > I think that's probably so. But my understanding is that in this case I don'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 that are unanswered or didn't even get proper attention for me to feel comfortable with the state of the RFC in question. Best regards, Gina P. Banyard [1] https://github.com/Girgias/php-rfcs/blob/master/comparison-equality-semantics.md [2] https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md
