[Truffle] Add special values of BigDecimal #2985
Conversation
| @@ -110,21 +168,23 @@ public RubyBasicObject initialize(RubyBasicObject self, RubyBasicObject v) { | |||
|
|
|||
| @Specialization(guards = "isRubyString(v)") | |||
There was a problem hiding this comment.
I would put @TruffleBoundary on this. The v.toString() is not going to compile well.
|
Looks great! |
| public RubyBasicObject initializeFromString(RubyBasicObject self, RubyBasicObject v) { | ||
| // TODO (pitr 20-May-2015): add NaN, Infinity handling | ||
| // TODO (pitr 21-May-2015): add profile? | ||
| // TODO (pitr 25-May-2015): construction of positive and negative zero | ||
| switch (v.toString()) { |
There was a problem hiding this comment.
How does this work? Is toString() redefined somehow for BigDecimal?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Ahh I missed the guard with Chris' note. I think it's clearer to have RubyString as the argument type in this case.
There was a problem hiding this comment.
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
force-pushed
the
bigdecimal
branch
2 times, most recently
from
0bdce40 to
72a6979
Compare
|
I had to add coerce specializations to other numerical classes, to make |
|
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. |
|
Yes |
|
But I think @pitr-ch meant |
|
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 |
|
great! |
|
This PR can be merged, assuming that I can ignore the two failing jobs on Travis. |
|
|
||
| def coerce(other) | ||
| [BigDecimal(other), self] | ||
| # TODO (pitr 28-may-2015): detect what cannot be coerced and raise? |
There was a problem hiding this comment.
BigDecimal() should raise so that implementation should be fine.
There was a problem hiding this comment.
Right, thanks, I'll remove it.
|
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. |
|
(it might be hard to rebase with the renames etc) |
|
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. |
- :+, :to_s, :nan?, :finite?, :zero?, :<=> adapted
…ions - special POSITIVE_ZERO removed, normal zero is used instead - handle big zero errors
not ready for merge