Skip to content

Commit

Permalink
Showing 2 changed files with 4 additions and 9 deletions.
1 change: 0 additions & 1 deletion spec/truffle/tags/core/bignum/coerce_tags.txt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -260,12 +260,12 @@ public abstract static class EqualNode extends CoreMethodArrayArgumentsNode {

@Specialization
public boolean equal(DynamicObject a, int b) {
return false;
return Layouts.BIGNUM.getValue(a).equals(BigInteger.valueOf(b));
}

@Specialization
public boolean equal(DynamicObject a, long b) {
return false;
return Layouts.BIGNUM.getValue(a).equals(BigInteger.valueOf(b));
}

@Specialization
@@ -465,19 +465,15 @@ public abstract static class CoerceNode extends CoreMethodArrayArgumentsNode {
public DynamicObject coerce(DynamicObject a, int b) {
CompilerDirectives.transferToInterpreter();

// TODO (eregon, 16 Feb. 2015): This is NOT spec, but let's try to see if we can make it work.
// b is converted to a Bignum here in other implementations.
Object[] store = new Object[] { b, a };
Object[] store = new Object[] { Layouts.BIGNUM.createBignum(coreLibrary().getBignumFactory(), BigInteger.valueOf(b)), a };
return Layouts.ARRAY.createArray(coreLibrary().getArrayFactory(), store, store.length);
}

@Specialization
public DynamicObject coerce(DynamicObject a, long b) {
CompilerDirectives.transferToInterpreter();

// TODO (eregon, 16 Feb. 2015): This is NOT spec, but let's try to see if we can make it work.
// b is converted to a Bignum here in other implementations.
Object[] store = new Object[] { b, a };
Object[] store = new Object[] { Layouts.BIGNUM.createBignum(coreLibrary().getBignumFactory(), BigInteger.valueOf(b)), a };
return Layouts.ARRAY.createArray(coreLibrary().getArrayFactory(), store, store.length);
}

10 comments on commit d7d6723

@bjfish
Copy link
Contributor Author

@bjfish bjfish commented on d7d6723 Apr 23, 2016

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

@chrisseaton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me.

@eregon
Copy link
Member

@eregon eregon commented on d7d6723 Apr 24, 2016

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?

Sorry, something went wrong.

@eregon
Copy link
Member

@eregon eregon commented on d7d6723 Apr 24, 2016

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.

Sorry, something went wrong.

@chrisseaton
Copy link
Contributor

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?

@eregon
Copy link
Member

@eregon eregon commented on d7d6723 Apr 25, 2016

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:

p 1 == (1<<77).coerce(1)[0]

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.

@chrisseaton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor for Bignum used to assert the value was outside long range

Yeah, but this turned out to be just wrong didn't it? We added that assumption.

@eregon
Copy link
Member

@eregon eregon commented on d7d6723 Apr 25, 2016

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.

@bjfish
Copy link
Contributor Author

@bjfish bjfish commented on d7d6723 Apr 25, 2016

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:

1 == ((1<<77).coerce(1)[0]) # should be true because Bignum 1 coerces to Fixnum 1
1.equal?((1<<77).coerce(1)[0]) # should be false because types don't match

@eregon
Copy link
Member

@eregon eregon commented on d7d6723 Apr 25, 2016

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.

Please sign in to comment.