-
Notifications
You must be signed in to change notification settings - Fork 305
Fix complex.__new__ #1385
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
Fix complex.__new__ #1385
Conversation
Could be an idea but we should probably check if it would be useful outside of the class myfloat(float):
def __float__(self):
return 1.0
assert float(myfloat(0.0)) == 1.0
assert abs(myfloat(0.0)) == 0.0 |
| } else if (value is int i) { | ||
| result = i; | ||
| } else if (value is BigInteger bi) { | ||
| result = (double)bi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these cast is where testing fails. The BigInteger to double cast returns infinity instead of overflowing. There's a MathUtils.ToFloat64 that throws (but we should consider a helper at the ipy level so we can match the error message).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. The fast track just cut too many corners. __float__ and __complex__ on int do it correctly, with a call to MathUtils.ToFloat64. Funny thing, I started working on complex.__new__ as a spin-off from an investigation whether int.__complex__ can be safely removed (it can, assuming that complex.__new__ works correctly) and then completely forgot about that.
I like the idea of our own helper class with a Python-specific error message.
|
I looked into the CPython's code and roughly speaking the functionality of |
slozier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
A thought: move
TryToFloattoPythonOps?