[Gnucash-changes] r13107 - gnucash/trunk - Some gnc-numeric fixes (more to come).

[Derek Atkins]

Author: warlord
Date: 2006-02-04 20:41:27 -0500 (Sat, 04 Feb 2006)
New Revision: 13107
Trac: http://svn.gnucash.org/trac/changeset/13107

Modified:
   gnucash/trunk/ChangeLog
   gnucash/trunk/lib/libqof/qof/gnc-numeric.c
Log:
Some gnc-numeric fixes (more to come).

	  - Fixes for handling reciprocal values.
	  - Mark a lot of places where potential overflow bugs
	    are not handled.



Modified: gnucash/trunk/ChangeLog
===================================================================
--- gnucash/trunk/ChangeLog	2006-02-05 00:16:38 UTC (rev 13106)
+++ gnucash/trunk/ChangeLog	2006-02-05 01:41:27 UTC (rev 13107)
@@ -1,3 +1,10 @@
+2006-02-04  Derek Atkins  <derek at ihtfp.com>
+
+	* lib/libqof/qof/gnc-numeric.c:
+	  - Fixes for handling reciprocal values.
+	  - Mark a lot of places where potential overflow bugs
+	    are not handled.
+
 2006-02-04  Joshua Sled  <jsled at asynchronous.org>
 
 	* src/doc/xml/gnucash-v2.rnc: Add reverse-engineered schema for

Modified: gnucash/trunk/lib/libqof/qof/gnc-numeric.c
===================================================================
--- gnucash/trunk/lib/libqof/qof/gnc-numeric.c	2006-02-05 00:16:38 UTC (rev 13106)
+++ gnucash/trunk/lib/libqof/qof/gnc-numeric.c	2006-02-05 01:41:27 UTC (rev 13107)
@@ -243,6 +243,14 @@
     return cmp128 (l,r);
   }
 
+  if (a.denom < 0)
+      a.denom *= -1;
+  if (b.denom < 0)
+      b.denom *= -1;
+
+  /* BUG: Possible overflow here..  Also, doesn't properly deal with
+   * reciprocal denominators.
+   */
   aa = a.num * a.denom;
   bb = b.num * b.denom;
 
@@ -270,6 +278,7 @@
 gboolean
 gnc_numeric_equal(gnc_numeric a, gnc_numeric b) 
 {
+  qofint128 l, r;
   if ((a.denom == b.denom) && (a.denom > 0))
   {
     return (a.num == b.num);
@@ -277,8 +286,8 @@
   if ((a.denom > 0) && (b.denom > 0))
   {
     // return (a.num*b.denom == b.num*a.denom);
-    qofint128 l = mult128 (a.num, b.denom);
-    qofint128 r = mult128 (b.num, a.denom);
+    l = mult128 (a.num, b.denom);
+    r = mult128 (b.num, a.denom);
     return equal128 (l, r);
 
 #if ALT_WAY_OF_CHECKING_EQUALITY
@@ -289,14 +298,24 @@
     return 1;
 #endif
   }
-  if ((a.denom < 0) && (b.denom < 0))
-  {    
-    return ((a.num * b.denom) == (a.denom * b.num));
+  if ((a.denom < 0) && (b.denom < 0)) {
+      l = mult128 (a.num, -a.denom);
+      r = mult128 (b.num, -b.denom);
+      return equal128 (l, r);
+  } else {
+      /* BUG: One of the numbers has a reciprocal denom, and the other
+         does not. I just don't know to handle this case in any
+         reasonably overflow-proof yet simple way.  So, this funtion
+         will simply get it wrong whenever the three multiplies
+         overflow 64-bits.  -CAS */
+      if (a.denom < 0) {
+          return ((a.num * -a.denom * b.denom) == b.num);
+      } else {
+          return (a.num == (b.num * a.denom * -b.denom));
+      }
   }
-  else 
-  {
-    return 0;
-  }
+
+  return ((a.num * b.denom) == (a.denom * b.num));
 }
 
 
@@ -355,20 +374,20 @@
   
   if(a.denom < 0) 
   {
-    a.num *= a.denom;
+    a.num *= -a.denom;  /* BUG: overflow not handled.  */
     a.denom = 1;
   }
 
   if(b.denom < 0) 
   {
-    b.num *= b.denom;
+    b.num *= -b.denom;  /* BUG: overflow not handled.  */
     b.denom = 1;
   }
 
   /* Get an exact answer.. same denominator is the common case. */ 
   if(a.denom == b.denom) 
   {
-    sum.num = a.num + b.num;
+    sum.num = a.num + b.num;  /* BUG: overflow not handled.  */
     sum.denom = a.denom;
   }
   else 
@@ -377,7 +396,7 @@
      *    sum.num = a.num*b.denom + b.num*a.denom;
      *    sum.denom = a.denom*b.denom;
      * but the multiply could overflow.  
-     * Computing the LCD minimizes likelyhood of overflow
+     * Computing the LCD minimizes likelihood of overflow
      */
     gint64 lcd;
     qofint128 ca, cb, cab;
@@ -468,12 +487,12 @@
   }
 
   if(a.denom < 0) {
-    a.num *= a.denom;
+    a.num *= -a.denom;  /* BUG: overflow not handled.  */
     a.denom = 1;
   }
 
   if(b.denom < 0) {
-    b.num *= b.denom;
+    b.num *= -b.denom;  /* BUG: overflow not handled.  */
     b.denom = 1;
   }
 
@@ -584,13 +603,13 @@
 
   if(a.denom < 0) 
   {
-    a.num *= a.denom;
+    a.num *= -a.denom;   /* BUG: overflow not handled.  */
     a.denom = 1;
   }
 
   if(b.denom < 0) 
   {
-    b.num *= b.denom;
+    b.num *= -b.denom;   /* BUG: overflow not handled.  */
     b.denom = 1;
   }
 
@@ -793,7 +812,7 @@
   
   /* If the denominator of the input value is negative, get rid of that. */
   if(in.denom < 0) {
-    in.num = in.num * (- in.denom);
+    in.num = in.num * (- in.denom);  /* BUG: overflow not handled.  */
     in.denom = 1;
   }
   
@@ -808,9 +827,9 @@
     denom     = - denom;
     denom_neg = 1;
     temp_a    = (in.num < 0) ? -in.num : in.num;
-    temp_bc   = in.denom * denom;
-    remainder = in.num % temp_bc;
-    out.num   = in.num / temp_bc;
+    temp_bc   = in.denom * denom;  /* BUG: overflow not handled.  */
+    remainder = temp_a % temp_bc;
+    out.num   = temp_a / temp_bc;
     out.denom = - denom;    
   }
   else 
@@ -1081,7 +1100,7 @@
   }
   else 
   {
-    return (double)(in.num * in.denom);
+    return (double)(in.num * -in.denom);
   }
 }