-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
max(new Decimal(NAN), new Decimal(-INF)) give not correct result (-INF) #14
Comments
The current behaviour around NAN is that any comparison returns 1. A comparison must return an integer, so PHP uses 1 if comparison is undefined. https://3v4l.org/84IC7 I don't think you should be using any php functions with this library. Many of them do not consider objects or will implicitly cast to float. I advise that you write your own function to achieve min/max. If we did implement min/max, we should do so on a standardized math library under the decimal namespace that always returns Decimal. This would include functions like min, max, sum, avg, sin, cos, tan, etc. |
Yes. For "usual" objects this is correct. |
It overrides the comparing behaviour of the Decimal object, but not comparison in PHP itself. This extension has no vision into the min or max functions. Comparing context is not known. |
I would like to see your wrapper, by the way. I'm curious to take a look. 😊 |
Sorry, i'm never going deep into PHP/extension internals (just not enough enough free time...) Therefore, i must add own min/max functions to wrapper. Thank you for answer. P.S. I'll send you wrapper and tests soon (monday, i suppose). Wrapper it self is not interesting. But tests. |
Feel free to close. :) PS: Check out the 2.0 branch also - some very nice performance improvements there. |
Hello, Rudi.
[MonetaryDecimal.zip](https://github.com/php-decimal/ext-decimal/files/2885565/MonetaryDecimal.zip)
Source attached (not sure it is pass github mailer, but try).
I hope you find something interesting for you.
For example, "execLambdaTest" in tests are interesting, as for me
(flexible thing, allow to test functions with any arguments count)
Writing same in
$this->assertSame((string)self::FLOAT_NEGATIVE, (new MonetaryDecimal(self::FLOAT_NEGATIVE))->toString());
style is much, much worst (long and bad readable). Especially, with many arguments.
But, this is just IMHO.
Anyway, enjoy. And thanks for you work with Decimal.
Best regards,
FoxKeys mailto:FoxKeys@Mail.ru
|
P.S.
Unfortunately, i can't. Our ITOps installs only pre-built production packages (pecl install decimal), so onl 1.3 are installed. |
@FoxKeys you will find significant benefits from using 2.0 by looking at your code. ✨ I'll try to get a 2.0.0-alpha released on PECL soon. |
As we discussed in ##10
NAN is not comparable, must raise and exception OR return NAN
Because signum() have INT result, you decide to raise an exception for signum() (and i agree with this).
But, in case of min/max return of NAN seems to be better solution.
Important! I'm not sure that min()/max() assumed to be used with Decimal at all.
If so - then this issue must be fixed.
If Decimal not allowed to be used with min()/max() functions - then raise and exception would be good if one (or both) of argumens for min/max is Decimal. And, if Decimal wil be forbidden argument for PHP min()/max() then new Decimal->min(...)/Decimal->max(...) would be good idea for Decimal API.
P.S. Just info to @rtheunissen. min/max is last test case in our project (at least for now), so no new issues expected from me :).
And, i can send you PHPUnit test file for our Decimal wrapper. Maybe you will find something interesting ;)
P.P.S. I'm pretty happy with your fast answer/fixes.
Best regards,
Sergey
The text was updated successfully, but these errors were encountered: