-
-
Notifications
You must be signed in to change notification settings - Fork 925
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- 9.4.12.0
- 9.4.11.0
- 9.4.10.0
- 9.4.9.0
- 9.4.8.0
- 9.4.7.0
- 9.4.6.0
- 9.4.5.0
- 9.4.4.0
- 9.4.3.0
- 9.4.2.0
- 9.4.1.0
- 9.4.0.0
- 9.3.15.0
- 9.3.14.0
- 9.3.13.0
- 9.3.12.0
- 9.3.11.0
- 9.3.10.0
- 9.3.9.0
- 9.3.8.0
- 9.3.7.0
- 9.3.6.0
- 9.3.5.0
- 9.3.4.0
- 9.3.3.0
- 9.3.2.0
- 9.3.1.0
- 9.3.0.0
- 9.2.21.0
- 9.2.20.1
- 9.2.20.0
- 9.2.19.0
- 9.2.18.0
- 9.2.17.0
- 9.2.16.0
- 9.2.15.0
- 9.2.14.0
- 9.2.13.0
- 9.2.12.0
- 9.2.11.1
- 9.2.11.0
- 9.2.10.0
- 9.2.9.0
- 9.2.8.0
- 9.2.7.0
- 9.2.6.0
- 9.2.5.0
- 9.2.4.1
- 9.2.4.0
- 9.2.3.0
- 9.2.2.0
- 9.2.1.0
- 9.2.0.0
- 9.1.17.0
- 9.1.16.0
- 9.1.15.0
- 9.1.14.0
- 9.1.13.0
- 9.1.12.0
- 9.1.11.0
- 9.1.10.0
- 9.1.9.0
- 9.1.8.0
- 9.1.7.0
- 9.1.6.0
- 9.1.5.0
- 9.1.4.0
- 9.1.3.0
- 9.1.2.0
- 9.1.1.0
- 9.1.0.0
Showing
2 changed files
with
4 additions
and
9 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d7d6723
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.
Is implementing this okay? Re: @eregon 's TODO that I removed above. cc: @chrisseaton
d7d6723
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.
Seems fine to me.
d7d6723
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.
@bjfish It breaks the clear delimitation that there is no Bignum in the Fixnum range. This makes working with Bignum/BigInteger a lot more dangerous, so I'd rather not have it and fix the spec. It seems essentially leaking implementation details of MRI.
Or is there an actual use case?
d7d6723
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.
About the consequences of
coerce
, this seems essentially harmless so far since anyway all core numeric types already handles all combinations, so it's not that helpful to have a coerce Fixnum => Bignum since Bignum operations already deal directly with Fixnum.d7d6723
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.
Maybe we should consult with the MRI people to see if they think this is a bug or not?
d7d6723
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.
@bjfish I will revert this since it breaks many assumptions and for instance code like:
The constructor for Bignum used to assert the value was outside
long
range, I'll try to add that back.For MRI, it seems known at the least but I'll discuss it on the mailing list.
d7d6723
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.
Yeah, but this turned out to be just wrong didn't it? We added that assumption.
d7d6723
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.
@chrisseaton No, it was fine and just got removed with the Layout stuff AFAIK.
d7d6723
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.
@eregon Why can't we implement this to match the MRI behavior? MRI appears to have Bignum in the Fixnum range.
I know the behavior may seem a little silly but that's what the coerce method was meant for.
The following is the MRI behavior. I think not implementing coerce to return a Bignum would make the second example fail:
d7d6723
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.
@bjfish Of course we can, but I think it's not a very worthy trade-off. I think this is an implementation detail of MRI and therefore has limited value in reproducing without further motivation. From my experience, having 2 possible representations for the same number just gets everything more complicated and causes more headaches to debug. Indeed, such a test would fail. But does it have any importance for users? (Also more instances having the same identity is rarely a problem, it also could be the case if some Bignum was cached)
I beg to differ about coerce 😄 Actually it should return arguments such that redoing the call/operator succeeds and return types are more general that can directly respond to the coerced values without further
coerce
(like a Fixnum and a Rational would create two Rationals).In the case of Fixnum/Bignum,
coerce
should never be needed as they implement all combinations anyway.And when Integer will eventually replace them then we can truly remove this coerce and Fixnum/Bignum altogether.