Re: [RFC] [Discussion] Support object type in BCMath
| From: | Saki Takamachi | 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