[Truffle] Add special values of BigDecimal

[pitr-ch]

not ready for merge

[chrisseaton]

I would put @TruffleBoundary on this. The v.toString() is not going to compile well.

[chrisseaton]

Looks great!

[eregon]

How does this work? Is toString() redefined somehow for BigDecimal?

[pitr-ch]

In short: badly 😞. Just saw https://github.com/pitr-ch/jruby/blob/bigdecimal/truffle/src/main/java/org/jruby/truffle/runtime/core/RubyBasicObject.java#L175-175 so I have to fix this. Thanks.

[eregon]

What you want is really v as a RubyString, right?
Then it makes sense to call toString() although it is not very efficient (good enough for now for debug output like this).

[eregon]

Ahh I missed the guard with Chris' note. I think it's clearer to have RubyString as the argument type in this case.

[chrisseaton]

RubyString#toString is the exception to that note. What we don't want to do is things like to_s as toString or hash as hatched - we don't want to map the Ruby API into the Java API like that (because those methods don't give us anywhere to store state for optimisation).

[pitr-ch]

I had to add coerce specializations to other numerical classes, to make > work. If that's ok I'll finish rest of the comparisons and operations with special values to make this merge-able.

[chrisseaton]

Looks good. Do you mean you'll add specialisations to classes like Float for comparing against BigDecimal? That doesn't sound right - as in MRI and Rubinius BigDecimal is a C extension and can't modify the other class methods like that. Look at the way Rational is handled - I think in some cases Fixnum > BigDecimal is done as BigDecimal <= Fixnum in some way.

[eregon]

Yes coerce should be used for these cases and should already be handled in Fixnum/Bignum/Float.

[eregon]

But I think @pitr-ch meant BigDecimal < Float for instance (in <=>), in which case it's fine to optimize that direction and probably even expected (to not need to call coerce).

[pitr-ch]

I mean changes like https://github.com/jruby/jruby/pull/2985/files#diff-03ee9d17196ada218812f3319c5c3f0c. It was either raising always an exception or missing completely. So for 1 > BigDecimal("0.5") it would raise argument error or missing node for FixnumNodes.Greater(int,BigDecimal). With this changes, it calls coerce on BigDecimal so 1 > BigDecimal("0.5") is translated to BigDecimal(1) > BigDecimal("0.5") and evaluated successfully. The other way BigDecimal("0.5") > 1 is done usually ending up in @Specialization(guards = "isNormal(a)") public int compare(RubyBasicObject a, int b) { return compareBigDecimal(a, getBigDecimalValue(b)); }

[eregon]

great!

[pitr-ch]

This PR can be merged, assuming that I can ignore the two failing jobs on Travis.

[eregon]

BigDecimal() should raise so that implementation should be fine.

[pitr-ch]

Right, thanks, I'll remove it.

[chrisseaton]

Great - yeah those failures aren't our problem - do you want to merge it yourself? Might as well rebase instead of creating a merge commit.

[eregon]

(it might be hard to rebase with the renames etc)

[pitr-ch]

Right I can push directly now :) I've already rebased this PR in the morning so I should not be more than few commits behind, rebase should be fine.