Apache OpenOffice (AOO) Bugzilla – Issue 86775
INT function gives an unexpected result due to too much precision
Last modified: 2013-08-07 15:15:24 UTC
The built-in INT function returns an unexpected result due to binary float-point rounding error when the value passed is very close to an integer. I'll attach an example file shortly.
Created attachment 51945 [details] test file that demonstrates the problem.
The first value (B1) returns 0.0 because the value being pushed to INT there is 0.999999999999999555910790, while the second value (B2) returns 1.0 because the pushed value is 1.000000000000000444089210. To minimize this problem, I propose reducing the precision to single-precision before flooring the value. It will not solve the fundamental problem entirely, but at least it will minimize the damage.
Created attachment 51946 [details] proposed patch
The patch isn't all that much. Basically it reduces the double value to float, back to double then floors it. That's all.
Created attachment 51948 [details] better patch - reduce precision only when the value is less than 1.0E10
Created attachment 51949 [details] test file to make sure the patch has not caused (obvious) regressions
Actually let's hold off on this. Reducing the precision will cause other weird problems down the road.
Meanwhile I'll just assign this to myself.
Created attachment 52270 [details] revised (probably final) patch
downstream issue: https://bugzilla.novell.com/show_bug.cgi?id=310706
@er: could you take a quick look at this patch and give me an approval? Thanks!
Created attachment 52272 [details] revised patch.. the previous patch is no good.
Created attachment 52273 [details] test file
In the test file, the red cells show values that were previously mis-calculated prior to applying this patch.
Kohei: Seems like .Net 2003 doesn't like "round": e:/build\2.4.0\src.pro\OOH680_m12\sc\source\core\tool\interpr1.cxx(109) : error C3861: 'round': identifier not found, even with argument-dependent lookup Please have a look.
Hi Kohei, Nice patch. Some small nitpick ;-) instead of if (::rtl::math::isNan(fValue) || ::rtl::math::isInf(fValue) || fValue == HUGE_VAL || fValue == 0.0) please use if (!::rtl::math::isFinite(fValue) || fValue == 0.0 || fValue == HUGE_VAL) isFinite() checks for both, NAN and INF, using the platform dependent SAL_MATH_FINITE(), and internally isNan() and and isInf() use the same but do extra work to determine whether it's a NAN or INF then, which is unnecessary here. For the round() we may use ::rtl::math::round() instead, that also allows control over the rounding mode, compiler libraries maybe differ in their internal implementation of round(). I think we might use floor(approxValue()) also in other places where up to now we use approxFloor(), what do you think? If so, we could add that as ::rtl::math::approxValue() and maybe should simply change the ::rtl::math::approxFloor() and ::rtl::math::approxCeil() methods to use that, and then would get rid of their threshhold behavior as well. Just a quick idea, could you imagine any negative side effect when doing so? Thanks Eike
Hi Eike, >could you imagine any negative side effect when doing so? Not off hand. I too think it's a good idea to call approxValue from within approxFloor() and approxCeil(). I can't think of any negatives since all the approx*() functions already truncate digits below 15 anyway. I'll change my patch to reflect this. I'll attach a new one when it's ready. Kohei
In cws odff03: sal/inc/rtl/math.h 1.3.380.1 sal/inc/rtl/math.hxx 1.6.380.1 sal/rtl/source/math.cxx 1.12.236.1 sal/util/sal.map 1.62.46.1 The patch needed some tweaking for values near DBL_MIN and DBL_MAX to not produce #NUM! errors. Added rtl_math_approxValue() and ::rtl::math::approxValue() wrapper; modified ::rtl::math::approxFloor() and ::rtl::math::approxCeil() to use ::rtl::math::approxValue(), hence no changes necessary in Calc code. Au contraire, also the FLOOR() and CEILING() spreadsheet functions and others using approxFloor()/approxCeil() internally actually benefit from this change because the dreaded 3.55271e-015 threshold value is gone now.
Ah, nice! Sorry I couldn't get to this soon enough. I have one question out of my ignorance. What is the sal.map file used for ? It looks like a symbol map for the exported symbols in the shared library, but is it used for all compilers or ... ? When do we need to modify this ?
Reassigning to QA for verification.
@kohei: Just returned from vacation, hence the long silence.. > What is the sal.map file used for ? It looks like a symbol map for the > exported symbols in the shared library, Exactly. All URE libraries use those map files, also each UNO component's library to export the few methods necessary for registry purposes. A few other libs that did not depend on the now removed SUPD numbering also use it. > but is it used for all compilers or ... ? Yes, used for all compilers. Platform specifics are handled in the tooling processing those files. It also provides a versioning mechanism. For details see http://udk.openoffice.org/common/man/libraryversioning.html Maybe a bit old, but basics didn't change. > When do we need to modify this ? Whenever you add a symbol that is to be exported ;-)
@er: ah thanks! I hope you enjoyed your vacation. Everyone needs a break every now and then. :-)
verified in internal build cws_odff3
closed because fix available in builds OOO300_m9 and DEV300_m33