[PATCH] Convert volume calculations into floating point

Linus Torvalds torvalds at linux-foundation.org
Wed Nov 5 15:36:19 PST 2014


On Wed, Nov 5, 2014 at 1:54 PM, Anton Lundin <glance at acc.umu.se> wrote:
> The basic problem was that for gases containing more than 2147483648 ml
> of nitrogen the calculations overflowed. This changes the code into
> using floating point math for that calculation which will be more
> accurate to.

Ugh. So many problems with this patch.

I agree that we do want to do part of it in floating point, but I
disagree with where you do it.

Doing the "1000 - get_he()" and "1000-o2" in floating point is just
silly, since that's pure integer math, with no chance of overflow or
even inexact.

And then, you really should use "rint()" to turn the floating point
value back into an integer value with proper rounding. Yeah, it
doesn't really matter when we talk "mliter" if we're off by one, but
it's the right thing to do.

Finally, while you do too *much* fp in the first line, you then don't
do *enough* floating point in the second one. You still do the
"vol.mliter*he" as an integer multiplication, which can overflow, and
do only the division as a FP value, which is pointless the way you do
it, since without rounding, you might as well just do an integer
divide in the first place. *With* rounding, it actually makes sense as
a floating point divide, but you don't have that "rint()", so you not
only don't avoid the overflow, the one FP op you do use is pointless.

So I think it should be something like

    air_vol = (vol.mliter * 1000.0) * (1000 - get_he(&mix) -
get_o2(&mix))) / (1000 - o2_in_topup);
    he_vol = (double) vol.mliter * get_he(&mix) / 1000.0;

    air.mliter = rint(air_vol * 1000);
    he.mliter = rint(he_vol * 1000);
    o2->mliter = vol.mliter - he->mliter - air.mliter;

so that *all* the multiples and divides get done as floating point,
but the exact integer subtractions don't, and the final integer
assignment is the properly rounded one.

FP is hard. But let's try to get it right.

Also, that whole "1000 - get_he(&mix) - get_o2(&mix)" might possibly
be good as a "get_n2(mix)" helper function. No? Although in this
context, maybe that doesn't make sense.

                          Linus


More information about the subsurface mailing list