vpiotr / decimal_for_cpp

Decimal data type for C++
273 stars 68 forks source link

Overflow in multDiv code #13

Closed azbobw closed 9 years ago

azbobw commented 9 years ago

There can be an overflow condition that isn't caught right now. Currently line 709 has: if ((abs(value1) <= DEC_MAX_INT32) || (abs(value2) <= DEC_MAX_INT32))

I encountered an overflow when using dec::decimal6. I changed my downloaded version and changed the line to:

if ((llabs(value1) <= DEC_MAX_INT32) && (llabs(value2) <= DEC_MAX_INT32))

The overflow occurred on the abs() call to one of the values. I thought it safer to make it an AND condition, not and OR.

vpiotr commented 9 years ago

You are correct, but I decided this part needs some rewrite, so it is corrected in a different way. Overflow issue is corrected, please test with your case.

azbobw commented 9 years ago

Thanks, will send tomorrow. I inherited from your class and had to change the /= operator to use (inefficient but effective) subtraction because 364.92 / 304.1001 was coming out with 1.2 and I needed it to be 1.199999. On Jul 12, 2015 4:40 AM, "vpiotr" notifications@github.com wrote:

Please provide test values for this issue. I don't see when it can be a problem.

— Reply to this email directly or view it on GitHub https://github.com/vpiotr/decimal_for_cpp/issues/13#issuecomment-120710754 .

azbobw commented 9 years ago

using dec::decimal6 as my type

1341.93115 / 10000.000000 was executed and caused me to find the issue

void test_abs(void) { dec::int64 resAbs, resllabs; dec::int64 val = 1341931150L;

resAbs = abs(val);
resllabs = llabs(val);

std::cout << std::endl << "  test_abs starting" << std::endl;

std::cout << "val : " << val << ", abs(): " << resAbs << ", llabs(): "

<< resllabs << std::endl;

val = 10000000000;
resAbs = abs(val);
resllabs = llabs(val);

std::cout << "val : " << val << ", abs(): " << resAbs << ", llabs(): "

<< resllabs << std::endl;

std::cout << std::endl << "  test_abs finished" << std::endl;
std::cout << std::endl;

return;

}

test_abs starting val : 1341931150, abs(): 1341931150, llabs(): 1341931150 val : 10000000000, abs(): 1410065408, llabs(): 10000000000

test_abs finished

exiting....

$ g++ -v Using built-in specs. Target: x86_64-redhat-linux Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux Thread model: posix gcc version 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC)

On Sun, Jul 12, 2015 at 12:50 PM, Bob Weidman azbweidman@gmail.com wrote:

Thanks, will send tomorrow. I inherited from your class and had to change the /= operator to use (inefficient but effective) subtraction because 364.92 / 304.1001 was coming out with 1.2 and I needed it to be 1.199999. On Jul 12, 2015 4:40 AM, "vpiotr" notifications@github.com wrote:

Please provide test values for this issue. I don't see when it can be a problem.

— Reply to this email directly or view it on GitHub https://github.com/vpiotr/decimal_for_cpp/issues/13#issuecomment-120710754 .

vpiotr commented 9 years ago

Please test with most recent version. For the first pair of values, correct result is in fact 1.2 (with rounding to 6 decimal places).

Without rounding result is equal to: 364.92 / 304.1001 = 1,1999996053930926033894760310832

azbobw commented 9 years ago

yes the answer is coming out 1.2. I'm reading code now to see where the rounding is taking place. I need to emulate the division without the rounding taking place. The program does rounding or truncation of digits to right of the decimal place at various locations - meaning at specific times. (Wants to retain the values as is until rounding or truncation of digits is desired.)

On Mon, Jul 13, 2015 at 5:26 AM, vpiotr notifications@github.com wrote:

Please test with most recent version. For the first pair of values, correct result is in fact 1.2 (with rounding to 6 decimal places).

Without rounding result is equal to: 364.92 / 304.1001 = 1,1999996053930926033894760310832

— Reply to this email directly or view it on GitHub https://github.com/vpiotr/decimal_for_cpp/issues/13#issuecomment-120909070 .

vpiotr commented 9 years ago

Currently you can switch off rounding for all class instances by the following steps:

I suppose it can be useful if I define rounding as a policy for this class, you can expect easier solution in the near future.

vpiotr commented 9 years ago

See BOOST_AUTO_TEST_CASE(decimalNoRoundPolicy) for no-rounding mode.

azbobw commented 9 years ago

Sorry for delay... Trying to download this and will test. Thanks for incorporating my change requests. On Jul 15, 2015 9:55 AM, "vpiotr" notifications@github.com wrote:

See BOOST_AUTO_TEST_CASE(decimalNoRoundPolicy) for no-rounding mode.

— Reply to this email directly or view it on GitHub https://github.com/vpiotr/decimal_for_cpp/issues/13#issuecomment-121678338 .