[RFC] [Discussion] Support object type in BCMath

Hi Barney,

No, that's not quite what I meant - I meant more like the opposite:

$bcNum = new BcNum('1234567890123456789.23456789');
echo $bcNum->format(8, '.', ',') // 1,234,567,890,123,456,789.23456789

Maybe also worth providing a way to specify that all decimals should be printed, instead of just a fixed number of decimals.

Ah I see! It sounds exactly like number_format. Sure, this might make sense. To me, this seems worth supporting.

BTW,

$bcNum = new BcNum('1234567890123456789.23456789’);

When I saw this, I thought that the behavior when $scale is omitted is that in addition to the option of "using the global setting value", there is also the option of "counting the length of $num and storing all $num”. In other words, it does not use any global settings.

Which of these do you think is better?

Regards.

Saki

Hi Aleksander,

After reading https://wiki.php.net/rfc/namespaces_in_bundled_extensions again I see it is a perfect case to apply it. While it's not a must, I think we should go with BCMath/Number.

Thank you, I have read it now. Certainly in this case it makes sense to use "BcMath" as the namespace ("BcMath" is appropriate instead of "BCMath" as it must follow PHP naming conventions).

What concerns me is that the symbol "Number" is difficult to understand in the code. So, how about "BcMath\BcNum”?

Regards.

Saki

Hi Aleksander,

If you write it as:

$result = $num->withScale(4)->add($num2);

it's not an extra line anymore. I also think that withScale() use will be rare, as we have the scale in constructor.

I think the intention is more clear here, and I think it applies to all cases you mentioned, including div or pow. If you know you need to change the scale just add ->withScale(X) before.

Ah, that's right. I wonder why I forgot about method chaining. Update RFC. If we miss something and $scale is needed, I can always revert the RFC.

I'm not sure I like this. Maybe we should be more strict here and treat the $scale in constructor (and later withScale()) as the actual scale for all operations.

So, in the case above I'd expect ->scale to be 2, and ->value to be '2.46'. If I wanted $num to have a scale that may change, I'd not define it in the first place. Does that make sense?

ps. that also means withScale(null) should be possible.

I see, that may indeed make sense. Now here we have three choices about it. In fact, if want to behave like the current RFC, we can do it by calculating as $num + new Number($str).
Yeah, I'll fix the RFC for this.

Regards.

Saki

$num = new BcNum('1.23', 2);
$result = $num + '1.23456';
$result->value; // '2.46456'
$result->scale; // ??

In this case, $result->scale will be '5'. I added this to the RFC.

I'm not sure I like this. Maybe we should be more strict here and treat
the $scale in constructor (and later withScale()) as the actual scale
for all operations.

For addition, it absolutely should expand scale like this, unless the
constructor also defines a default rounding type that is used in that
situation. All numbers, while arbitrary, will be finite, so addition will
always be exact and known based on inputs prior to calculation.

Treating scale like this isn't more strict, it's confusing. For instance:

$numA = new Number('1.23', 2);
$numB = new Number('1.23456', 5);

$expandedScale1 = $numA + $numB; // 2.46456
$expandedScale2 = $numB + $numA; // 2.46456

$strictScale1 = $numA + $numB; // 2.46 assuming truncation
$strictScale2 = $numB + $numA; // 2.46456

I ran into this same issue with operand ordering when I was writing my
operator overload RFC.

There are ways you could do the overload implementation that would get
around this for object + object operations, but it's also mathematically
unsound and probably unexpected for anyone who is going to the trouble of
using an arbitrary precision library.

Addition and subtraction should automatically use the largest scale from
all operands. Division and multiplication should require a specified scale.

Because of this, I'm not entirely sure that specifying a scale in the
constructor is actually a good thing. It is incredibly easy to create
situations, unless the implementation in C is VERY careful, where the
operand positions matter beyond the simple calculation. Multiplication is
commutative, but division is not. This would almost certainly lead to some
very difficult to track down bugs.

Putting scale in the constructor is similar to some of the examples of
"possible misuse cases of operator overloading" that I had to go over when
I was making my RFC. We definitely want to avoid that if possible for the
first number/math object that has operator overloads.

Jordan

Hi Derick,

What is the main difference between getting a read-only property with
->value and getting the value using a method?

Feeling :-) Do we have precedence with other extension's objects
perhaps already?

It depends on the extension. Probably some readonly properties exist in mysqli for example. (However, those in mysqli are only readonly internally, and stubs are not readonly. Therefore, many IDEs do not display errors, so w e don't realize we've written incorrect code until runtime.)
The Readonly property was implemented since PHP 8.1, so inner classes implemented before then did not have the option of using the readonly property. As far as I know, no new inner classes with readonly properties or simple getters have been added since PHP 8.1. Therefore, there is currently no precedent that can be compared on the same terms as BCMath\Number….

bcfloor and bcceil originally have no scale specification. This is
because the result is always a string representing an integer value.
And since the supported round-mode is the same as standard-round,
ROUND_FLOOR and ROUND_CEILING are also supported. Therefore, if
want to obtain floor or ceil behavior with a specified scale, I
recommend specifying the mode as round.

OK. That makes sense.

Note that we have removed $scale from the calculation method arguments, reflecting the discussion on the mailing list :)

• In this example, what would $result->scale show? (Perhaps add that to
  the example?):

<?php
$num = new BcNum('1.23', 2);
$result = $num + '1.23456';
$result->value; // '2.46456'
$result->scale; // ??

In this case, $result->scale will be '5'. I added this to the RFC.

Great.

This has also been changed to reflect discussion, with slightly different results.

$num = new BcNum('1.23', 2);
$result = $num + '1.23456';
$result->value; // '2.46'
$result->scale; // 2

It's what we did for the Date extension, and the Random extension, but
in this case, it's probably not needed as you say.

For that, yes. ValueErrors should be distinct from DivideByZeroError — I
think we do have both of those already:

php -r 'echo 8/0;'

Fatal error: Uncaught DivisionByZeroError: Division by zero in Command
line code on line 1

From the docs for ValueError:

"A ValueError is thrown when the type of an argument is correct but the
value of it is incorrect."

From the docs for DivisionByZeroError:

" DivisionByZeroError is thrown when an attempt is made to divide a
number by zero. "

Subclassing these for BCMath objects seems unnecessary therefore.

Thanks. Sorry, I meant "If I create dedicated exception classes, do I need separate classes for each type of error?” I couldn't write it well in English.

Regards.

Saki

On Thu, Apr 4, 2024 at 1:59 PM Barney Laurance barney@redmagic.org.uk
wrote:

Hi again,

Do we also need toFloat and toInt functions? Seems like using explicit functions will be safer than casting.

For toInt I'd expect an exception if the value is outside the range of possible ints. For toFloat it might be nice to have a flag
argument to give the developer the choice of having it throw if the value is outside the range of floats or return INF or -INF,
or possibly the user should just check for infinite values themselves.

I was thinking about those features too. However, I'm concerned that proposing too many features will complicate the RFC and make it difficult to get it approved.

Coming back to this point, I think these are basic features that people
would expect to be there - I think I would find just slightly frustrating
to start learning how to use a class like this and then
find that it doesn't have these functions. Casting and calling intval or
floatval all feel like slightly awkward workarounds that shouldn't be
needed in a greenfield project. We know that the string
inside the object is always a numeric string, so it should be easier to
parse it as an int than to parse it as a date or a JSON document. Code
doing the latter should stand out as odd looking.

The class cannot guarantee that it can return a value in the type you
request however, so the way that is handled would need to be decided. The
value can easily be outside of the range of an int. Should it return a
float silently in that case for toInt()? What if the value is beyond the
range of a float? That would be a very rare situation, as floats can
represent extremely large numbers (with very reduced accuracy), but I would
expect it to throw an exception if that happened. Ideally an exception that
I could catch and ignore, since I can almost surely deal with that error in
most situations.

What about a number that is so small that it can't fit in a float? Similar
situation, though I expect it would occur slightly more often than a number
being too large to fit in a float, even though it would also be rare.

I think these helper functions belong in the RFC, but they aren't quite
straightforward, which is what I think Saki was alluding to.

Jordan

Hi Rowan,

That's why I mentioned the two different groups of users. The scale and rounding mode aren't there for group (a), who just want the scale to be managed automatically; they are there for group (b), who want to guarantee a particular result has a particular scale. The result of $a->add($b, 2, Round::HALF_UP) will always be the same as $a->add($b)->round(Round::HALF_UP) but is more convenient, and in some cases more efficient, since it doesn't calculate unnecessary digits.

Remember also the title and original aim of the RFC: add object support to BCMath. The scale parameter is already there on the existing functions (bcadd, bcmul, etc), so removing it on the object version would be surprising. The rounding mode is a new feature, but there doesn't seem a good reason not to include it everywhere as well.

Ah, I understand. There may certainly be use cases where demand specifies a scale smaller than the maximum scale in order to save computational costs. That makes sense to me.

Also, considering the two groups you presented, the group that wants to control the scale is probably not going to use operator calculations, even though they could provide various options in the constructor. Unless look at the contents of objects in the calculation process using var_dump or something, can't understand the state just by looking at the code, which can lead to bugs. They probably don't like that kind of code.

Again, the aim was to match the functionality of the existing functions. It's likely that users will migrate code written using bcdiv() to use BCMath\Number->div() and expect it to work the same, at least when specifying a scale. Having it behave differently by rounding up the last digit by default seems like a bad idea.

Thinking about the implementation, the truncation behaviour also makes sense: the library isn't actually rounding anything, it's calculating digit by digit, and stopping when it reaches the requested scale.

The whole concept of rounding is something that we are adding, presumably by passing $scale+1 to the underlying library functions. It's a nice feature to add, but not one that should be on by default, given we're not writing the extension from scratch.

I was thinking about this today, and I think both are correct opinions on whether to set the initial value to HALF_UP or TOWARD_ZERO. It's just a matter of prioritizing whether consistency with existing behavior or consistency within a class, and they can never be met simultaneously.

Therefore, I am considering adding a more detailed explanation to the RFC on this issue and taking a second vote to decide.

Regards.

Saki