Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for BigDecimal#/ #797

Merged
merged 1 commit into from

2 participants

@tychobrailleur
Collaborator

This fixes #644 and #648. See this #648 (comment) comment for discussion.

@BanzaiMan
Owner

@tychobrailleur Could you rebase to the current master and squash these commits into one?

Thanks!

@tychobrailleur tychobrailleur Fix BigDecimal#/
When dividing BigDecimal by a Float, the Float is converted to
Rational “for higher precision,” and then divides the numerator
by the denominator.  However, when doing that division, `div19`
was being used which does a `floor` on the returned float.  This
commit replaces the call to `div19` with `op_div`.
This commit also addresses issues with return type of the result,
which changes from version to version in Ruby.
See rubyspec/rubyspec#220 for tests.

Use java.math.BigDecimal to compute rational and float value.

Make test applicable for 1.9/2.0 only.

Code cleanup.

A bit of housekeeping, adding whitespaces after commas, and around
equals.

Return Float when BigDecimal is divided by Float in 1.9.

Recent versions of 1.9.3 now exhibit the same behaviour as 2.0.
b5069f8
@BanzaiMan BanzaiMan merged commit 0447f28 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 17, 2013
  1. @tychobrailleur

    Fix BigDecimal#/

    tychobrailleur authored tychobrailleur committed
    When dividing BigDecimal by a Float, the Float is converted to
    Rational “for higher precision,” and then divides the numerator
    by the denominator.  However, when doing that division, `div19`
    was being used which does a `floor` on the returned float.  This
    commit replaces the call to `div19` with `op_div`.
    This commit also addresses issues with return type of the result,
    which changes from version to version in Ruby.
    See rubyspec/rubyspec#220 for tests.
    
    Use java.math.BigDecimal to compute rational and float value.
    
    Make test applicable for 1.9/2.0 only.
    
    Code cleanup.
    
    A bit of housekeeping, adding whitespaces after commas, and around
    equals.
    
    Return Float when BigDecimal is divided by Float in 1.9.
    
    Recent versions of 1.9.3 now exhibit the same behaviour as 2.0.
This page is out of date. Refresh to see the latest.
View
172 src/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -90,42 +90,42 @@ public IRubyObject allocate(Ruby runtime, RubyClass klass) {
public final static int ROUND_FLOOR = BigDecimal.ROUND_FLOOR;
@JRubyConstant
- public final static int SIGN_POSITIVE_INFINITE=3;
+ public final static int SIGN_POSITIVE_INFINITE = 3;
@JRubyConstant
- public final static int EXCEPTION_OVERFLOW=8;
+ public final static int EXCEPTION_OVERFLOW = 8;
@JRubyConstant
- public final static int SIGN_POSITIVE_ZERO=1;
+ public final static int SIGN_POSITIVE_ZERO = 1;
@JRubyConstant
- public final static int EXCEPTION_ALL=255;
+ public final static int EXCEPTION_ALL = 255;
@JRubyConstant
- public final static int SIGN_NEGATIVE_FINITE=-2;
+ public final static int SIGN_NEGATIVE_FINITE = -2;
@JRubyConstant
- public final static int EXCEPTION_UNDERFLOW=4;
+ public final static int EXCEPTION_UNDERFLOW = 4;
@JRubyConstant
- public final static int SIGN_NaN=0;
+ public final static int SIGN_NaN = 0;
@JRubyConstant
- public final static int BASE=10000;
+ public final static int BASE = 10000;
@JRubyConstant
- public final static int ROUND_MODE=256;
+ public final static int ROUND_MODE = 256;
@JRubyConstant
- public final static int SIGN_POSITIVE_FINITE=2;
+ public final static int SIGN_POSITIVE_FINITE = 2;
@JRubyConstant
- public final static int EXCEPTION_INFINITY=1;
+ public final static int EXCEPTION_INFINITY = 1;
@JRubyConstant
- public final static int SIGN_NEGATIVE_INFINITE=-3;
+ public final static int SIGN_NEGATIVE_INFINITE = -3;
@JRubyConstant
- public final static int EXCEPTION_ZERODIVIDE=1;
+ public final static int EXCEPTION_ZERODIVIDE = 1;
@JRubyConstant
- public final static int SIGN_NEGATIVE_ZERO=-1;
+ public final static int SIGN_NEGATIVE_ZERO = -1;
@JRubyConstant
- public final static int EXCEPTION_NaN=2;
+ public final static int EXCEPTION_NaN = 2;
// Static constants
private static final BigDecimal TWO = new BigDecimal(2);
private static final double SQRT_10 = 3.162277660168379332;
public static RubyClass createBigDecimal(Ruby runtime) {
- RubyClass bigDecimal = runtime.defineClass("BigDecimal",runtime.getNumeric(), BIGDECIMAL_ALLOCATOR);
+ RubyClass bigDecimal = runtime.defineClass("BigDecimal", runtime.getNumeric(), BIGDECIMAL_ALLOCATOR);
runtime.getKernel().defineAnnotatedMethods(BigDecimalKernelMethods.class);
@@ -433,7 +433,7 @@ private static boolean isOverflowExceptionMode(Ruby runtime) {
}
private static RubyBigDecimal cannotBeCoerced(ThreadContext context, IRubyObject v, boolean must) {
- if(must) {
+ if (must) {
String err;
if (v.isImmediate()) {
@@ -461,47 +461,23 @@ private static RubyBigDecimal getVpValue19(ThreadContext context, IRubyObject v,
}
private static IRubyObject getVpRubyObjectWithPrec19Inner(ThreadContext context, RubyRational r, long precision, boolean must) {
- RubyRational orig = null;
- IRubyObject numerator = null;
- while (true) {
- IRubyObject value;
- boolean div;
- if (orig == null) {
- orig = r;
- div = true;
- } else {
- div = orig != r;
- }
-
- if (div) {
- numerator = r.numerator(context);
- RubyBigDecimal pv = getVpValueWithPrec19(context, numerator, -1, must);
-
- if (pv == null) cannotBeCoerced(context, r, must);
-
- value = pv.div19(context, r.denominator(context)); // FIXME: This should propagate precision
- } else {
- value = orig;
- }
+ long numerator = RubyNumeric.num2long(r.numerator(context));
+ long denominator = RubyNumeric.num2long(r.denominator(context));
- if (value instanceof RubyFloat) value = ((RubyFloat) value).to_r(context);
-
- if (!(value instanceof RubyRational)) return value;
-
- r = (RubyRational) value;
- }
+ return new RubyBigDecimal(context.runtime,
+ BigDecimal.valueOf(numerator).divide(BigDecimal.valueOf(denominator)));
}
private static RubyBigDecimal getVpValueWithPrec19(ThreadContext context, IRubyObject value, long precision, boolean must) {
while (true) {
if (value instanceof RubyFloat) {
-// if (precision < 0) return unableToCoerceWithoutPrec(context, value, must);
if (precision > Long.MAX_VALUE) cannotBeCoerced(context, value, must);
- value = getVpRubyObjectWithPrec19Inner(context, (RubyRational) ((RubyFloat) value).to_r(context), precision, must); // MRI uses built-in
+ RubyFloat f = (RubyFloat)value;
+ value = new RubyBigDecimal(context.runtime, BigDecimal.valueOf(f.getDoubleValue()));
continue;
} else if (value instanceof RubyRational) {
- if (precision < 0) return unableToCoerceWithoutPrec(context, value, must);
+ if (precision < 0 && !context.runtime.is2_0()) return unableToCoerceWithoutPrec(context, value, must);
value = getVpRubyObjectWithPrec19Inner(context, (RubyRational) value, precision, must);
continue;
@@ -509,7 +485,7 @@ private static RubyBigDecimal getVpValueWithPrec19(ThreadContext context, IRubyO
return (RubyBigDecimal) value;
} else if (value instanceof RubyFixnum || value instanceof RubyBignum) {
String s = value.toString();
- return newInstance(value.getRuntime().getClass("BigDecimal"),new IRubyObject[]{value.getRuntime().newString(s)});
+ return newInstance(value.getRuntime().getClass("BigDecimal"), new IRubyObject[]{value.getRuntime().newString(s)});
}
return cannotBeCoerced(context, value, must);
@@ -517,11 +493,11 @@ private static RubyBigDecimal getVpValueWithPrec19(ThreadContext context, IRubyO
}
private static RubyBigDecimal getVpValue(IRubyObject v, boolean must) {
- if(v instanceof RubyBigDecimal) {
+ if (v instanceof RubyBigDecimal) {
return (RubyBigDecimal)v;
- } else if(v instanceof RubyFixnum || v instanceof RubyBignum) {
+ } else if (v instanceof RubyFixnum || v instanceof RubyBignum) {
String s = v.toString();
- return newInstance(v.getRuntime().getClass("BigDecimal"),new IRubyObject[]{v.getRuntime().newString(s)});
+ return newInstance(v.getRuntime().getClass("BigDecimal"), new IRubyObject[]{v.getRuntime().newString(s)});
}
return cannotBeCoerced(v.getRuntime().getCurrentContext(), v, must);
@@ -640,15 +616,14 @@ private static RubyBigDecimal newZero(Ruby runtime, int sign) {
} else {
zeroSign = 1;
}
- RubyBigDecimal rbd = new RubyBigDecimal(runtime, BigDecimal.ZERO, 0, zeroSign);
+ RubyBigDecimal rbd = new RubyBigDecimal(runtime, BigDecimal.ZERO, 0, zeroSign);
return rbd;
}
private static RubyBigDecimal newNaN(Ruby runtime) {
- if (isNaNExceptionMode(runtime)) {
- throw runtime.newFloatDomainError("Computation results to 'NaN'(Not a Number)");
- }
- RubyBigDecimal rbd = new RubyBigDecimal(runtime, BigDecimal.ZERO, true);
+ if (isNaNExceptionMode(runtime)) throw runtime.newFloatDomainError("Computation results to 'NaN'(Not a Number)");
+
+ RubyBigDecimal rbd = new RubyBigDecimal(runtime, BigDecimal.ZERO, true);
return rbd;
}
@@ -660,9 +635,8 @@ private static RubyBigDecimal newInfinity(Ruby runtime, int sign) {
infinitySign = 1;
}
RubyBigDecimal rbd = new RubyBigDecimal(runtime, BigDecimal.ZERO, infinitySign);
- if (isInfinityExceptionMode(runtime)) {
- throw runtime.newFloatDomainError("Computation results to 'Infinity'");
- }
+ if (isInfinityExceptionMode(runtime)) throw runtime.newFloatDomainError("Computation results to 'Infinity'");
+
return rbd;
}
@@ -672,9 +646,9 @@ private RubyBigDecimal setResult() {
private RubyBigDecimal setResult(int scale) {
int prec = RubyFixnum.fix2int(getRuntime().getClass("BigDecimal").searchInternalModuleVariable("vpPrecLimit"));
- int prec2 = Math.max(scale,prec);
- if(prec2 > 0 && this.value.scale() > (prec2-getExponent())) {
- this.value = this.value.setScale(prec2-getExponent(),BigDecimal.ROUND_HALF_UP);
+ int prec2 = Math.max(scale, prec);
+ if (prec2 > 0 && this.value.scale() > (prec2-getExponent())) {
+ this.value = this.value.setScale(prec2-getExponent(), BigDecimal.ROUND_HALF_UP);
}
return this;
}
@@ -750,7 +724,7 @@ public IRubyObject remainder(ThreadContext context, IRubyObject arg) {
if (isInfinity() || isNaN()) {
return newNaN(runtime);
}
- RubyBigDecimal val = getVpValue(arg,false);
+ RubyBigDecimal val = getVpValue(arg, false);
if (val == null) {
return callCoerced(context, "remainder", arg, true);
}
@@ -772,8 +746,8 @@ public IRubyObject op_mul(ThreadContext context, IRubyObject arg) {
public IRubyObject mult2(ThreadContext context, IRubyObject b, IRubyObject n) {
Ruby runtime = context.runtime;
- RubyBigDecimal val = getVpValue(b,false);
- if(val == null) {
+ RubyBigDecimal val = getVpValue(b, false);
+ if (val == null) {
// TODO: what about n arg?
return callCoerced(context, "*", b);
}
@@ -979,20 +953,20 @@ public IRubyObject op_uplus() {
@JRubyMethod(name = "-", required = 1)
public IRubyObject op_minus(ThreadContext context, IRubyObject arg) {
RubyBigDecimal val = getVpValue(arg, false);
- if(val == null) {
+ if (val == null) {
return callCoerced(context, "-", arg);
}
RubyBigDecimal res = handleMinusSpecialValues(val);
if (res != null) {
return res;
}
- return new RubyBigDecimal(getRuntime(),value.subtract(val.value)).setResult();
+ return new RubyBigDecimal(getRuntime(), value.subtract(val.value)).setResult();
}
@JRubyMethod(name = "sub", required = 2)
public IRubyObject sub2(ThreadContext context, IRubyObject b, IRubyObject n) {
RubyBigDecimal val = getVpValue(b, false);
- if(val == null) {
+ if (val == null) {
return callCoerced(context, "-", b);
}
RubyBigDecimal res = handleMinusSpecialValues(val);
@@ -1000,7 +974,7 @@ public IRubyObject sub2(ThreadContext context, IRubyObject b, IRubyObject n) {
return res;
}
- return new RubyBigDecimal(getRuntime(),value.subtract(val.value)).setResult();
+ return new RubyBigDecimal(getRuntime(), value.subtract(val.value)).setResult();
}
private RubyBigDecimal handleMinusSpecialValues(RubyBigDecimal val) {
@@ -1049,15 +1023,31 @@ public IRubyObject op_uminus() {
public IRubyObject op_quo(ThreadContext context, IRubyObject other) {
// regular division with some default precision
// TODO: proper algorithm to set the precision
- return op_div(context, other, getRuntime().newFixnum(200));
+ return convertDivResult(context, other,
+ op_div(context, other, getRuntime().newFixnum(200)));
}
@JRubyMethod(name = {"/", "quo"}, compat = CompatVersion.RUBY1_9)
- public IRubyObject op_quo19(ThreadContext context, IRubyObject other) {
- other = getVpValue19(context, other, true);
+ public IRubyObject op_quo19(ThreadContext context, IRubyObject other) {
+ return op_quo19_20(context, other);
+ }
+
+ @JRubyMethod(name = {"/", "quo"}, compat = CompatVersion.RUBY2_0)
+ public IRubyObject op_quo20(ThreadContext context, IRubyObject other) {
+ return op_quo19_20(context, other);
+ }
+
+ private IRubyObject op_quo19_20(ThreadContext context, IRubyObject other) {
+ RubyObject preciseOther = getVpValue19(context, other, true);
// regular division with some default precision
// TODO: proper algorithm to set the precision
- return op_div(context, other, getRuntime().newFixnum(200));
+ return op_div(context, preciseOther, getRuntime().newFixnum(200));
+ }
+
+ private IRubyObject convertDivResult(ThreadContext context, IRubyObject other, IRubyObject result) {
+ if (other instanceof RubyFloat && result instanceof RubyBigDecimal)
+ result = ((RubyBigDecimal)result).convertToFloat();
+ return result;
}
@JRubyMethod(name = "div", compat = CompatVersion.RUBY1_8)
@@ -1164,10 +1154,10 @@ public IRubyObject op_div19(ThreadContext context, IRubyObject other, IRubyObjec
private IRubyObject cmp(ThreadContext context, IRubyObject r, char op) {
int e = 0;
- RubyBigDecimal rb = getVpValue(r,false);
- if(rb == null) {
- IRubyObject ee = callCoerced(context, "<=>",r);
- if(ee.isNil()) {
+ RubyBigDecimal rb = getVpValue(r, false);
+ if (rb == null) {
+ IRubyObject ee = callCoerced(context, "<=>", r);
+ if (ee.isNil()) {
if (op == '*') {
return getRuntime().getNil();
} else if (op == '=' || isNaN()){
@@ -1202,33 +1192,33 @@ private IRubyObject cmp(ThreadContext context, IRubyObject r, char op) {
@Override
@JRubyMethod(name = "<=>", required = 1)
public IRubyObject op_cmp(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'*');
+ return cmp(context, arg, '*');
}
@Override
@JRubyMethod(name = {"eql?", "==", "==="}, required = 1)
public IRubyObject eql_p(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'=');
+ return cmp(context, arg, '=');
}
@JRubyMethod(name = "<", required = 1)
public IRubyObject op_lt(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'<');
+ return cmp(context, arg, '<');
}
@JRubyMethod(name = "<=", required = 1)
public IRubyObject op_le(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'L');
+ return cmp(context, arg, 'L');
}
@JRubyMethod(name = ">", required = 1)
public IRubyObject op_gt(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'>');
+ return cmp(context, arg, '>');
}
@JRubyMethod(name = ">=", required = 1)
public IRubyObject op_ge(ThreadContext context, IRubyObject arg) {
- return cmp(context, arg,'G');
+ return cmp(context, arg, 'G');
}
@JRubyMethod(name = "abs")
@@ -1285,10 +1275,10 @@ public IRubyObject ceil19(IRubyObject[] args) {
@Override
public IRubyObject coerce(IRubyObject other) {
IRubyObject obj;
- if(other instanceof RubyFloat) {
- obj = getRuntime().newArray(other,to_f());
+ if (other instanceof RubyFloat) {
+ obj = getRuntime().newArray(other, to_f());
} else {
- obj = getRuntime().newArray(getVpValue(other, true),this);
+ obj = getRuntime().newArray(getVpValue(other, true), this);
}
return obj;
}
@@ -1515,7 +1505,7 @@ public IRubyObject round(ThreadContext context, IRubyObject[] args) {
// -1 -> 10's digit, -2 -> 100's digit, etc.
BigDecimal normalized = value.movePointRight(scale);
// ...round to that digit
- BigDecimal rounded = normalized.setScale(0,mode);
+ BigDecimal rounded = normalized.setScale(0, mode);
// ...and shift the result back to the left (multiply by 10**(abs(scale)))
return new RubyBigDecimal(getRuntime(), rounded.movePointLeft(scale));
} else {
@@ -1698,7 +1688,7 @@ public IRubyObject to_int19() {
private String removeTrailingZeroes(String in) {
while(in.length() > 0 && in.charAt(in.length()-1)=='0') {
- in = in.substring(0,in.length()-1);
+ in = in.substring(0, in.length()-1);
}
return in;
}
@@ -1886,7 +1876,7 @@ public IRubyObject to_s(IRubyObject[] args) {
}
return getRuntime().newString(zero);
}
- if(asEngineering(arg)) {
+ if (asEngineering(arg)) {
return engineeringValue(arg);
} else {
return floatingPointValue(arg);
@@ -1966,7 +1956,7 @@ public static BigDecimal bigSqrt(BigDecimal squarD, MathContext rootMC) {
int sign = squarD.signum();
if (sign == -1) {
throw new ArithmeticException("Square root of a negative number: " + squarD);
- } else if(sign == 0) {
+ } else if (sign == 0) {
return squarD.round(rootMC);
}
View
16 test/test_big_decimal.rb
@@ -247,8 +247,20 @@ def test_decimal_format
#JRUBY-5190
def test_large_precisions
a = BigDecimal("1").div(BigDecimal("3"), 307)
- b = BigDecimal("1").div(BigDecimal("3") , 308)
+ b = BigDecimal("1").div(BigDecimal("3"), 308)
assert_equal a.to_f, b.to_f
end
-
+
+ if RUBY_VERSION =~ /1\.9/ || RUBY_VERSION =~ /2\.0/
+ # GH-644, GH-648
+ def test_div_by_float_precision_gh644
+ a = BigDecimal.new(11023)/2.2046
+ assert_equal 5_000, a.to_f
+ end
+
+ def test_div_by_float_precision_gh648
+ b = BigDecimal.new(1.05, 10)/1.48
+ assert (b.to_f - 0.7094594594594595) < Float::EPSILON
+ end
+ end
end
Something went wrong with that request. Please try again.