Use double rather than long double in oj_num_as_value

[BuonOmo]

Fixes #538.

It seems that double are already enough, and better handled by ruby
internals:

VALUE working() {
  long double d = 5.23683071;
  return rb_float_new((double)d);
}

VALUE not_working() {
  long double d = 5.23683071L;
  return rb_float_new((double)d);
}

void Init_main() {
  rb_define_global_function("working", working, 0);
  rb_define_global_function("not_working", not_working, 0);
}

working returns the correct value, not_working returns a floating
point issued stuff (5.2368307099999996).

Moreover, long double are not widely supported in ruby, for instance in
vsnprintf.c, line 528:

#define	LONGDBL		0x008		/* long double; unimplemented */

Or the ffi lib doesn't seem to like it either: ffi/ffi#194

[ohler55]

I'll have to do more testing on this reversion. The original Oj code used a double and I changed it to long double to handle rounding issues. I'll go back over the close issues and see if I can find the values that prompted the change. I might not have added those values to unit tests.

[BuonOmo]

If there is still a rounding issue, it may be possible to cast before the call to pow as you hinted in the linked issue:

diff --git a/ext/oj/parse.c b/ext/oj/parse.c
index 23ab726..e5974a2 100644
--- a/ext/oj/parse.c
+++ b/ext/oj/parse.c
@@ -795,13 +795,13 @@ oj_num_as_value(NumInfo ni) {
 	    }
 	} else {
 	    // All these machinations are to get rounding to work better.
-	    double d = (double)ni->i * (double)ni->div + (double)ni->num;
+	    long double ld = (long double)ni->i * (long double)ni->div + (long double)ni->num;
 	    int	x = (int)((int64_t)ni->exp - ni->di);
 
 	    // Rounding sometimes cuts off the last digit even if there are only
 	    // 15 digits. This attempts to fix those few cases where this
 	    // occurs.
-	    if ((double)INT64_MAX > d && (int64_t)d != (ni->i * ni->div + ni->num)) {
+	    if ((long double)INT64_MAX > ld && (int64_t)ld != (ni->i * ni->div + ni->num)) {
 		volatile VALUE	bd = rb_str_new(ni->str, ni->len);
 
 		rnum = rb_rescue2(parse_big_decimal, bd, rescue_big_decimal, bd, rb_eException, 0);
@@ -809,7 +809,8 @@ oj_num_as_value(NumInfo ni) {
 		    rnum = rb_funcall(rnum, rb_intern("to_f"), 0);
 		}
 	    } else {
-		d = round(d);
+		double d = roundl(ld);
+
 		if (0 < x) {
 		    d *= pow(10.0, x);
 		} else if (0 > x) {

[ohler55]

It would be helpful to know when the change in the string version occurs. Is it in the call to round or pow.

[BuonOmo]

The pow division of a long double definitely looks like it is the issue:

// main.c
#include <ruby/ruby.h>

#define dbg(v) rb_funcall(Qnil, rb_intern("p"), 1, rb_float_new((double)v))

VALUE testing() {
  long double ld = 523683071.0L;
  double d = ld;
  dbg(ld);
  dbg(d);
  ld = roundl(ld);
  d = roundl(ld);
  dbg(ld);
  dbg(d);
  d /= pow(10.0, 8);
  /*
  ld /= powl(10.0L, 8);
  /*/
  ld /= pow(10.0, 8);
  //*/
  dbg(ld);
  dbg(d);
  return Qnil;
}

void Init_main() {
  rb_define_global_function("testing", testing, 0);
}

# extconf.rb
require 'mkmf'
create_makefile('main')

$ ruby extconf.rb && make && ruby -r ./main -e 'testing'
creating Makefile
linking shared-object main.bundle
523683071.0
523683071.0
523683071.0
523683071.0
5.2368307099999996
5.23683071

I indeed found the long double commit, but not sign of any related test nor comment unfortunately. a93ffad

[ohler55]

Thanks for finding the original change and the debugging. I would expect to use powl for long double though. If the commented out code is used is there a difference?

[BuonOmo]

There is no difference in the result. Feel free to play with this code as well.

We're kind of stuck in my company: I had a day to fix floating point issues and decided to take it to fix the source. Could you tell me if and when you think you'll be able to merge such a PR? I'd be glad to help more if I could. However, I think all the evidences are here!

[ohler55]

I'm going to merge but make some changes and add unit tests.

[BuonOmo]

Thanks a lot for the quick release, we're already using it in production! I'd be glad to work with you again :)