Re: [RFC] [Discussion] Support object type in BCMath

From: Date: Mon, 08 Apr 2024 00:08:56 +0000
Subject: Re: [RFC] [Discussion] Support object type in BCMath
References: 1 2 3 4 5 6  Groups: php.internals 
Request: Send a blank email to internals+get-123027@lists.php.net to get a copy of this message
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 “stubâ€� 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 a¾Ø; > Œ±‰¿‰º}A/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

« previous php.internals (#123027) next »