Fixes to Time and BigDecimal for #3616.

* BigDecimal#to_r was not producing correct results. * Time.at was not attempting to coerce using to_r. * Time.at rational logic was over-complicated. Fixes #3616 and untags a few specs.
jruby · Jan 26, 2016 · 16b13e1 · 16b13e1
1 parent 25600ba
commit 16b13e1
Showing 6 changed files with 82 additions and 66 deletions.
diff --git a/core/src/main/java/org/jruby/RubyRational.java b/core/src/main/java/org/jruby/RubyRational.java
@@ -189,6 +189,11 @@ private static RubyRational newRationalBang(ThreadContext context, IRubyObject c
     private static RubyRational newRationalBang(ThreadContext context, IRubyObject clazz, IRubyObject x) {
         return newRationalBang(context, clazz, x, RubyFixnum.one(context.runtime));
     }
+
+    @Override
+    public ClassIndex getNativeClassIndex() {
+        return ClassIndex.RATIONAL;
+    }
 
     private IRubyObject num;
     private IRubyObject den;
@@ -405,6 +410,14 @@ public IRubyObject denominator(ThreadContext context) {
         return den;
     }
 
+    public IRubyObject getNumerator() {
+        return num;
+    }
+
+    public IRubyObject getDenominator() {
+        return den;
+    }
+
     /** f_imul
      * 
      */

diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
@@ -82,8 +82,8 @@
 @JRubyClass(name="Time", include="Comparable")
 public class RubyTime extends RubyObject {
     public static final String UTC = "UTC";
-    @Deprecated // no longer used
     public static final BigDecimal ONE_MILLION_BD = BigDecimal.valueOf(1000000);
+    public static final BigDecimal ONE_BILLION_BD = BigDecimal.valueOf(1000000000);
     public static final BigInteger ONE_MILLION_BI = BigInteger.valueOf(1000000);
     public static final BigDecimal ONE_THOUSAND_BD = BigDecimal.valueOf(1000);
     private DateTime dt;
@@ -259,29 +259,59 @@ public static DateTimeZone getTimeZoneFromUtcOffset(Ruby runtime, IRubyObject ut
     // mri: time.c num_exact
     private static IRubyObject numExact(Ruby runtime, IRubyObject v) {
         IRubyObject tmp;
-        if (v instanceof RubyFixnum || v instanceof RubyBignum) return v;
-        if (v.isNil()) exactTypeError(runtime, v);
-        if (!(v instanceof RubyRational)) { // Default unknown
-            if (v.respondsTo("to_r")) {
-                tmp = v.callMethod(runtime.getCurrentContext(), "to_r");
-                // WTF is this condition for?  It responds to to_r and makes something which thinks it is a String?
-                if (tmp != null && v.respondsTo("to_str")) exactTypeError(runtime, v);
-            } else {
-                tmp = TypeConverter.checkIntegerType(runtime, v, "to_int");
-                if (tmp.isNil()) exactTypeError(runtime, v);
-            }
-            v = tmp;
+        boolean typeError = false;
+
+        switch (v.getMetaClass().getClassIndex()) {
+            case FIXNUM:
+            case BIGNUM:
+                return v;
+
+            case RATIONAL:
+                break;
+
+            case STRING:
+            case NIL:
+                typeError = true;
+                break;
+
+            default:
+                if ((tmp = v.getMetaClass().finvokeChecked(runtime.getCurrentContext(), v, "to_r")) != null) {
+                    /* test to_int method availability to reject non-Numeric
+                     * objects such as String, Time, etc which have to_r method. */
+                    if (!v.respondsTo("to_int")) {
+                        typeError = true;
+                        break;
+                    }
+                    v = tmp;
+                    break;
+                }
+                if (!(tmp = TypeConverter.checkIntegerType(runtime, v, "to_int")).isNil()) {
+                    v = tmp;
+                    break;
+                }
+                typeError = true;
+                break;
         }
 
-        if (v instanceof RubyFixnum || v instanceof RubyBignum) {
-            return v;
-        } else if (v instanceof RubyRational) {
-            RubyRational r = (RubyRational) v;
-            if (r.denominator(runtime.getCurrentContext()) == RubyFixnum.newFixnum(runtime, 1)) {
-                return r.numerator(runtime.getCurrentContext());
-            }
-        } else {
-            exactTypeError(runtime, v);
+        switch (v.getMetaClass().getClassIndex()) {
+            case FIXNUM:
+            case BIGNUM:
+                return v;
+
+            case RATIONAL:
+                if (((RubyRational) v).getDenominator() == RubyFixnum.one(runtime)) {
+                    v = ((RubyRational) v).getNumerator();
+                }
+                break;
+
+            default:
+                typeError = true;
+                break;
+        }
+
+        if (typeError) {
+            if (v.isNil()) throw runtime.newTypeError("can't convert nil into an exact number");
+            throw runtime.newTypeError("can't convert " + v.getMetaClass() + " into an exact number");
         }
 
         return v;
@@ -1032,6 +1062,8 @@ public static IRubyObject at(ThreadContext context, IRubyObject recv, IRubyObjec
             long nanosecs;
             long millisecs;
 
+            arg = numExact(runtime, arg);
+
             // In the case of two arguments, MRI will discard the portion of
             // the first argument after a decimal point (i.e., "floor").
             // However in the case of a single argument, any portion after
@@ -1057,23 +1089,15 @@ public static IRubyObject at(ThreadContext context, IRubyObject recv, IRubyObjec
                     RubyRational rational = (RubyRational) arg;
 
                     // These could have rounding errors if numerator or denominator are not integral and < long. Can they be?
-                    long numerator = rational.numerator(context).convertToInteger().getLongValue();
-                    long denominator = rational.denominator(context).convertToInteger().getLongValue();
+                    long numerator = rational.getNumerator().convertToInteger().getLongValue();
+                    long denominator = rational.getDenominator().convertToInteger().getLongValue();
 
-                    final BigDecimal secs;
-                    if ( numerator <= Integer.MAX_VALUE ) {
-                        secs = new BigDecimal(numerator * 1000);
-                    }
-                    else {
-                        secs = BigDecimal.valueOf(numerator).multiply(ONE_THOUSAND_BD);
-                    }
-                    final BigDecimal millis = secs.divide(BigDecimal.valueOf(denominator), 12, RoundingMode.DOWN);
-
-                    final BigInteger roundMillis = millis.toBigInteger();
-                    BigInteger remainingNanos = millis.movePointRight(6).toBigInteger().subtract( roundMillis.multiply(ONE_MILLION_BI) );
+                    BigDecimal nanosBD = BigDecimal.valueOf(numerator).divide(BigDecimal.valueOf(denominator), 50, BigDecimal.ROUND_HALF_UP).multiply(ONE_BILLION_BD);
+                    BigInteger millis = nanosBD.divide(ONE_MILLION_BD).toBigInteger();
+                    BigInteger nanos = nanosBD.remainder(ONE_MILLION_BD).toBigInteger();
 
-                    millisecs = roundMillis.longValue();
-                    nanosecs = remainingNanos.longValue();
+                    millisecs = millis.longValue();
+                    nanosecs = nanos.longValue();
                 }
             } else {
                 nanosecs = 0;
@@ -1109,6 +1133,9 @@ public static IRubyObject at(ThreadContext context, IRubyObject recv, IRubyObjec
         long millisecs;
         long nanosecs = 0;
 
+        arg1 = numExact(runtime, arg1);
+        arg2 = numExact(runtime, arg2);
+
         if (arg1 instanceof RubyFloat || arg1 instanceof RubyRational) {
             double dbl = RubyNumeric.num2dbl(arg1);
             millisecs = (long) (dbl * 1000);

diff --git a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -1518,31 +1518,11 @@ public IRubyObject to_int() {
     public IRubyObject to_r(ThreadContext context) {
         checkFloatDomain();
 
-        RubyArray i = split(context);
-        long sign = (long)i.get(0);
-        String digits = (String)i.get(1).toString();
-        long base = (long)i.get(2);
-        long power = (long)i.get(3);
-        long denomi_power = power - digits.length();
-
-        IRubyObject bigDigits = RubyBignum.newBignum(getRuntime(), (String)digits).op_mul(context, sign);
-        RubyBignum numerator;
-        if(bigDigits instanceof RubyBignum) {
-          numerator = (RubyBignum)bigDigits;
-        }
-        else {
-          numerator = RubyBignum.newBignum(getRuntime(), bigDigits.toString());
-        }
-        IRubyObject num, den;
-        if(denomi_power < 0) {
-            num = numerator;
-            den = RubyFixnum.newFixnum(getRuntime(), base).op_mul(context, RubyFixnum.newFixnum(getRuntime(), -denomi_power));
-        }
-        else {
-            num = numerator.op_pow(context, RubyFixnum.newFixnum(getRuntime(), base).op_mul(context, RubyFixnum.newFixnum(getRuntime(), denomi_power)));
-            den = RubyFixnum.newFixnum(getRuntime(), 1);
-        }
-        return RubyRational.newInstance(context, context.runtime.getRational(), num, den);
+        int scale = value.scale();
+        BigInteger numerator = value.scaleByPowerOfTen(scale).toBigInteger();
+        BigInteger denominator = BigInteger.valueOf((long)Math.pow(10, scale));
+
+        return RubyRational.newInstance(context, context.runtime.getRational(), RubyBignum.newBignum(context.runtime, numerator), RubyBignum.newBignum(context.runtime, denominator));
     }
 
     public IRubyObject to_int19() {

diff --git a/spec/tags/ruby/core/time/at_tags.txt b/spec/tags/ruby/core/time/at_tags.txt
diff --git a/spec/tags/ruby/library/bigdecimal/mode_tags.txt b/spec/tags/ruby/library/bigdecimal/mode_tags.txt
diff --git a/spec/tags/ruby/library/bigdecimal/new_tags.txt b/spec/tags/ruby/library/bigdecimal/new_tags.txt
@@ -1,2 +1 @@
 critical(JRUBY-3749):BigDecimal.new doesn't segfault when using a very large string to build the number
-fails:BigDecimal.new creates a new object of class BigDecimal
Original file line number	Diff line number	Diff line change
		@@ -1,2 +1 @@
		critical(JRUBY-3749):BigDecimal.new doesn't segfault when using a very large string to build the number
		fails:BigDecimal.new creates a new object of class BigDecimal