Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122758
Date: Tue, 26 Mar 2024 15:47:56 +0000 (GMT)
To: Saki Takamachi <saki@sakiot.com>
cc: php internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Support object type in BCMath
In-Reply-To: <4F094EDA-5058-407D-AF39-06FD934FDE1F@sakiot.com>
Message-ID: <bbcff629-4dd6-bc50-ac9f-641849dc218a@php.net>
References: <4F094EDA-5058-407D-AF39-06FD934FDE1F@sakiot.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
From: derick@php.net (Derick Rethans)

On Sun, 24 Mar 2024, Saki Takamachi wrote:

> Hi internals,
> 
> I want to start the discussion on the PHP RFC: Support object type in BCMath.
> 
> https://wiki.php.net/rfc/support_object_type_in_bcmath

I have some comments:

- You've picked as class name "BcNum". Following 
  our naming guidelines, that probably should be \BCMath\Num (or 
  \BC\Num, but that is less descriptive): 
  https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#namespaces-in-extensions

  The reason it *should* have "BC" is that it comes from "Basic 
  Calculator" (https://www.php.net/manual/en/book.bc.php#118203)

- Should ->value rather be ->toString() ? ->value alone doesn't really 
  say much. I'm on the fence here though, as there is already 
  (internally) a ->__toString() method to make the (string) cast work.

- Would it make sense to have "floor" and "ceil" to also have a scale, 
  or precision? Or would developers instead have to use "round" in that 
  case?

- Which rounding modes are supported with "round", the same ones as the 
  normal round() function?

- 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; // ??

- Exceptions

  The RFC does not mention which exceptions can be thrown. Is it just 
  the one? It might be beneficial to *do* have a new exception 
  hierarchy.



cheers,
Derick

-- 
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social