Skip to content
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

Is signum(NAN) === 1 conform IEEE? #10

Closed
FoxKeys opened this issue Feb 1, 2019 · 7 comments
Closed

Is signum(NAN) === 1 conform IEEE? #10

FoxKeys opened this issue Feb 1, 2019 · 7 comments

Comments

@FoxKeys
Copy link

FoxKeys commented Feb 1, 2019

Have a question.
->signum(NAN) returns 1.
Is this correct and conform IEEE? Or, maybe, must throw an exception?

@FoxKeys FoxKeys changed the title Is signum(NAN) conform IEEE? Is signum(NAN) === 1 conform IEEE? Feb 1, 2019
@rtheunissen
Copy link
Contributor

I believe it should return NaN. I'm not aware of any definition around this by IEEE, but it seems like all other implementations return NaN.

@FoxKeys
Copy link
Author

FoxKeys commented Feb 1, 2019

I have found this

@rtheunissen
Copy link
Contributor

rtheunissen commented Feb 1, 2019

That is the library we're using. 😊 I don't see any mention of sign in the docs though.

The only mention of sign in the source is in these functions:

/* 0 if dec is positive, 1 if dec is negative */
ALWAYS_INLINE uint8_t
mpd_sign(const mpd_t *dec)
{
    return dec->flags & MPD_NEG;
}

/* 1 if dec is positive, -1 if dec is negative */
ALWAYS_INLINE int
mpd_arith_sign(const mpd_t *dec)
{
    return 1 - 2 * mpd_isnegative(dec);
}

It makes sense that NaN returns NaN because it is not zero, not positive, and not negative. Throwing an exception seems strange though - the caller should handle the NaN according to its domain.

@FoxKeys
Copy link
Author

FoxKeys commented Feb 1, 2019

That is the library we're using.

I know, but i can't understand this: "result is set to NaN and INT_MAX is returned."
So, INT_MAX is returned? But how result is used then?

If signum return NAN - it can be a problem, because function signature is" ->signum(): int", but NAN is not int. Or i mistake?

@rtheunissen
Copy link
Contributor

I know, but i can't understand this: "result is set to NaN and INT_MAX is returned."

That is the compare function, not signum. It's saying that NaN compared to anything else (including another NaN) is not defined. Unrelated to sign, I believe.

If signum return NAN - it can be a problem, because function signature is" ->signum(): int", but NAN is not int. Or i mistake?

You are absolutely right, which makes it tricky. We have some options here because the 2.0 branch is just about ready to go.

A) Change the return type to ?int, returning null for NaN. (I don't like this very much).
B) Throw an exception for NaN.
C) Return 1 for NaN.
D) Remove the return type and return float NAN for NaN.

I'm leaning towards B because there is value in being able to rely on an integer return value, and it seems extremely unlikely that querying the sign of NaN is intended. If that is the case, the caller can catch the exception and do nothing with it. This puts the responsibility on the caller/domain to handle NaN, rather than the library.

Something like this:

if (UNEXPECTED(mpd_isnan(mpd)) {
    php_decimal_sign_of_nan_not_defined();
    return;
}

@FoxKeys
Copy link
Author

FoxKeys commented Feb 1, 2019

I'm not expert in decimal math, but option "B" seems best for PHP-way.
It is predictable and testable, IMHO

@rtheunissen
Copy link
Contributor

This has been fixed in b1e0162 and released as v1.2.0 ✨

Throws RuntimeException for NAN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants