[Scummvm-cvs-logs] SF.net SVN: scummvm:[55839] scummvm/trunk

[fingolfin at users.sourceforge.net]

Revision: 55839
          http://scummvm.svn.sourceforge.net/scummvm/?rev=55839&view=rev
Author:   fingolfin
Date:     2011-02-09 00:12:02 +0000 (Wed, 09 Feb 2011)

Log Message:
-----------
COMMON: Reduce overflow risk in Common::Rational += and -= operators

Modified Paths:
--------------
    scummvm/trunk/common/rational.cpp
    scummvm/trunk/test/common/rational.h

Modified: scummvm/trunk/common/rational.cpp
===================================================================
--- scummvm/trunk/common/rational.cpp	2011-02-09 00:11:39 UTC (rev 55838)
+++ scummvm/trunk/common/rational.cpp	2011-02-09 00:12:02 UTC (rev 55839)
@@ -57,7 +57,7 @@
 	// Cancel the fraction by dividing both the num and the denom
 	// by their greatest common divisor.
 
-	int gcd = Common::gcd(_num, _denom);
+	const int gcd = Common::gcd(_num, _denom);
 
 	_num   /= gcd;
 	_denom /= gcd;
@@ -78,18 +78,30 @@
 }
 
 Rational &Rational::operator+=(const Rational &right) {
-	_num   = _num * right._denom + right._num * _denom;
-	_denom = _denom * right._denom;
+	// Cancel common factors to avoid unnecessary overflow.
+	// Note that the result is *not* always normalized.
+	const int gcd = Common::gcd(_denom, right._denom);
 
+	_num    = _num * (right._denom / gcd);
+	_denom  = _denom / gcd;
+	_num   += right._num * _denom;
+	_denom *= right._denom;
+
 	cancel();
 
 	return *this;
 }
 
 Rational &Rational::operator-=(const Rational &right) {
-	_num   = _num * right._denom - right._num * _denom;
-	_denom = _denom * right._denom;
+	// Cancel common factors to avoid unnecessary overflow.
+	// Note that the result is *not* always normalized.
+	const int gcd = Common::gcd(_denom, right._denom);
 
+	_num    = _num * (right._denom / gcd);
+	_denom  = _denom / gcd;
+	_num   -= right._num * _denom;
+	_denom *= right._denom;
+
 	cancel();
 
 	return *this;
@@ -98,8 +110,8 @@
 Rational &Rational::operator*=(const Rational &right) {
 	// Cross-cancel to avoid unnecessary overflow;
 	// the result then is automatically normalized
-	int gcd1 = Common::gcd(_num, right._denom);
-	int gcd2 = Common::gcd(right._num, _denom);
+	const int gcd1 = Common::gcd(_num, right._denom);
+	const int gcd2 = Common::gcd(right._num, _denom);
 
 	_num   = (_num    / gcd1) * (right._num    / gcd2);
 	_denom = (_denom  / gcd2) * (right._denom  / gcd1);

Modified: scummvm/trunk/test/common/rational.h
===================================================================
--- scummvm/trunk/test/common/rational.h	2011-02-09 00:11:39 UTC (rev 55838)
+++ scummvm/trunk/test/common/rational.h	2011-02-09 00:12:02 UTC (rev 55839)
@@ -86,6 +86,22 @@
 		TS_ASSERT_EQUALS(r1 - 1, Common::Rational(-1, 2));
 	}
 
+	void test_add_sub2() {
+		// Make sure cancelation works correctly
+		const Common::Rational r0(4, 15);	// = 8 / 30
+		const Common::Rational r1(1, 6);	// = 5 / 30
+
+		TS_ASSERT_EQUALS(r0 + r1, Common::Rational(13, 30));
+		TS_ASSERT_EQUALS(r1 + r0, Common::Rational(13, 30));
+		TS_ASSERT_EQUALS(r0 - r1, Common::Rational(1, 10));
+		TS_ASSERT_EQUALS(r1 - r0, Common::Rational(-1, 10));
+
+		TS_ASSERT_EQUALS(1 + r1, Common::Rational(7, 6));
+		TS_ASSERT_EQUALS(r1 + 1, Common::Rational(7, 6));
+		TS_ASSERT_EQUALS(1 - r1, Common::Rational(5, 6));
+		TS_ASSERT_EQUALS(r1 - 1, Common::Rational(-5, 6));
+	}
+
 	void test_mul() {
 		const Common::Rational r0(6, 3);
 		const Common::Rational r1(1, 2);


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.