[Boost-commit] svn:boost r85526 - in trunk: boost libs/rational libs/rational/test

Subject: [Boost-commit] svn:boost r85526 - in trunk: boost libs/rational libs/rational/test
From: dwalker07_at_[hidden]
Date: 2013-08-30 09:18:38

Author: dlwalker
Date: 2013-08-30 09:18:37 EDT (Fri, 30 Aug 2013)
New Revision: 85526
URL: http://svn.boost.org/trac/boost/changeset/85526

Log:
Fixed normalization problem with Boost.Rational's "assign"; add check for negative values too large for normalization (refs #9067)

Text files modified: 
   trunk/boost/rational.hpp                   |    16 ++++++++++----                          
   trunk/libs/rational/rational.html          |    41 ++++++++++++++++++++++++++++++--------- 
   trunk/libs/rational/test/rational_test.cpp |    41 ++++++++++++++++++++++++++++++++++++++++
   3 files changed, 83 insertions(+), 15 deletions(-)

Modified: trunk/boost/rational.hpp
==============================================================================
--- trunk/boost/rational.hpp	Fri Aug 30 05:04:40 2013	(r85525)
+++ trunk/boost/rational.hpp	2013-08-30 09:18:37 EDT (Fri, 30 Aug 2013)	(r85526)
@@ -21,6 +21,8 @@
 //    Nickolay Mladenov, for the implementation of operator+=
 
 //  Revision History
+//  30 Aug 13  Improve exception safety of "assign"; start modernizing I/O code
+//             (Daryle Walker)
 //  27 Aug 13  Add cross-version constructor template, plus some private helper
 //             functions; add constructor to exception class to take custom
 //             messages (Daryle Walker)
@@ -258,10 +260,7 @@
 template <typename IntType>
 inline rational<IntType>& rational<IntType>::assign(param_type n, param_type d)
 {
-    num = n;
-    den = d;
-    normalize();
-    return *this;
+    return *this = rational( n, d );
 }
 
 // Unary plus and minus
@@ -571,6 +570,13 @@
         den = -den;
     }
 
+    // ...But acknowledge that the previous step doesn't always work.
+    // (Nominally, this should be done before the mutating steps, but this
+    // member function is only called during the constructor, so we never have
+    // to worry about zombie objects.)
+    if (den < zero)
+        throw bad_rational( "bad rational: non-zero singular denominator" );
+
     BOOST_ASSERT( this->test_invariant() );
 }
 
@@ -623,7 +629,7 @@
 {
     using namespace std;
 
-    // The slash directly preceeds the denominator, which has no prefixes.
+    // The slash directly precedes the denominator, which has no prefixes.
     ostringstream  ss;
 
     ss.copyfmt( os );

Modified: trunk/libs/rational/rational.html
==============================================================================
--- trunk/libs/rational/rational.html	Fri Aug 30 05:04:40 2013	(r85525)
+++ trunk/libs/rational/rational.html	2013-08-30 09:18:37 EDT (Fri, 30 Aug 2013)	(r85526)
@@ -345,12 +345,19 @@
 type.
 
 <h3><a name="In-place assignment">In-place assignment</a></h3>
-For any <tt>rational&lt;I&gt; r</tt>, <tt>r.assign(n, m)</tt> provides a
-fast equivalent of <tt>r = rational&lt;I&gt;(n, m);</tt>, without the
+For any <tt>rational&lt;I&gt; r</tt>, <tt>r.assign(n, m)</tt> provides an
+alternate to <tt>r = rational&lt;I&gt;(n, m);</tt>, without a user-specified
 construction of a temporary. While this is probably unnecessary for rationals
 based on machine integer types, it could offer a saving for rationals based on
 unlimited-precision integers, for example.
 
+<p>The function will throw if the given components cannot be formed into a valid
+rational number.  Otherwise, it could throw only if the component-level move
+assignment (in C++11; copy-assignment for earlier C++ versions) can throw.  The
+strong guarantee is kept if throwing happens in the first part, but there is a
+risk of neither the strong nor basic guarantees happening if an exception is
+thrown during the component assignments.
+
 <h3><a name="Conversions">Conversions</a></h3>
 <p>There is a conversion operator to an unspecified Boolean type (most likely a
 member pointer).  This operator converts a rational to <code>false</code> if it
@@ -499,15 +506,17 @@
 <h2><a name="Exceptions">Exceptions</a></h2>
 Rationals can never have a denominator of zero. (This library does not support
 representations for infinity or NaN). Should a rational result ever generate a
-denominator of zero, the exception <tt>boost::bad_rational</tt> (a subclass of
-<tt>std::domain_error</tt>) is thrown. This should only occur if the user
-attempts to explicitly construct a rational with a denominator of zero, or to
-divide a rational by a zero value.  The exception can also be thrown during a
-cross-instantiation conversion, when at least one of the components ends up not
-being value-preserved and the new combination is not considered normalized.
+denominator of zero, or otherwise fail during normalization, the exception
+<tt>boost::bad_rational</tt> (a subclass of <tt>std::domain_error</tt>) is
+thrown. This should only occur if the user attempts to explicitly construct a
+rational with a denominator of zero, to divide a rational by a zero value, or
+generate a negative denominator too large to be normalized.  The exception can
+be thrown during a cross-instantiation conversion, when at least one of the
+components ends up not being value-preserved and the new combination is not
+considered normalized.
 
 <p>In addition, if operations on the underlying integer type can generate
-exceptions, these will be propogated out of the operations on the rational
+exceptions, these will be propagated out of the operations on the rational
 class. No particular assumptions should be made - it is only safe to assume
 that any exceptions which can be thrown by the integer class could be thrown
 by any rational operation. In particular, the rational constructor may throw
@@ -516,6 +525,11 @@
 only throw exceptions which can be thrown by the destructor of the underlying
 integer type (usually none).
 
+<p>If the component-level assignment operator(s) can throw, then a rational
+object's invariants may be violated if an exception happens during the second
+component's assignment.  (The <code>assign</code> member function counts here
+too.)  This violates both the strong and basic guarantees.
+
 <h2><a name="Internal representation">Internal representation</a></h2>
 <em>Note:</em> This information is for information only. Programs should not
 be written in such a way as to rely on these implementation details.
@@ -570,6 +584,13 @@
 magnitude-dependent granularity is typical of floating point representations.
 However, it does not "feel" natural when using a rational number class.
 
+<p>Limited-precision integer types may raise issues with the range sizes of
+their allowable negative values and positive values.  If the negative range is
+larger, then the extremely-negative numbers will not have an additive inverse in
+the positive range, making them unusable as denominator values since they cannot
+be normalized to positive values (unless the user is lucky enough that the input
+components are not relatively prime pre-normalization).
+
 <p>It is up to the user of a rational type based on a limited-precision integer
 type to be aware of, and code in anticipation of, such issues.
 
@@ -704,7 +725,7 @@
 be used in the same Boolean contexts as the built-in numeric types, in December
 2005.  He added the cross-instantiation constructor template in August 2013.
 
-<p>Revised August 27, 2013</p>
+<p>Revised August 30, 2013</p>
 
 <p>&copy; Copyright Paul Moore 1999-2001; &copy; Daryle Walker 2005, 2013.
 Permission to copy, use, modify, sell and distribute this document is granted

Modified: trunk/libs/rational/test/rational_test.cpp
==============================================================================
--- trunk/libs/rational/test/rational_test.cpp	Fri Aug 30 05:04:40 2013	(r85525)
+++ trunk/libs/rational/test/rational_test.cpp	2013-08-30 09:18:37 EDT (Fri, 30 Aug 2013)	(r85526)
@@ -19,6 +19,8 @@
 // since he hasn't been in contact for years.)
 
 // Revision History
+// 30 Aug 13  Add bug-test of assignments holding the basic and/or strong
+//            guarantees (Daryle Walker)
 // 27 Aug 13  Add test for cross-version constructor template (Daryle Walker)
 // 23 Aug 13  Add bug-test of narrowing conversions during order comparison;
 //            spell logical-negation in it as "!" because MSVC won't accept
@@ -1084,4 +1086,43 @@
     BOOST_REQUIRE( !dummy );
 }
 
+// "rational::assign" doesn't even have the basic guarantee
+BOOST_AUTO_TEST_CASE( ticket_9067_test )
+{
+    using boost::rational;
+    using boost::math::gcd;
+
+    rational<int>  a( 6, -8 );
+
+    // Normalize to maintain invariants
+    BOOST_CHECK_EQUAL( a.numerator(), -3 );
+    BOOST_CHECK_EQUAL( a.denominator(), 4 );
+    BOOST_CHECK( a.denominator() > 0 );
+    BOOST_CHECK_EQUAL( gcd(a.numerator(), a.denominator()), 1 );
+
+    // Do we maintain the basic guarantee after a failed component-assign?
+    BOOST_CHECK_THROW( a.assign(1, 0), boost::bad_rational );
+    BOOST_CHECK_NE( a.denominator(), 0 );
+    BOOST_CHECK( a.denominator() > 0 );
+    BOOST_CHECK_EQUAL( gcd(a.numerator(), a.denominator()), 1 );
+
+    // Do we get the strong guarantee?
+    BOOST_CHECK_EQUAL( a.numerator(), -3 );
+    BOOST_CHECK_EQUAL( a.denominator(), 4 );
+
+#if INT_MIN + INT_MAX < 0
+    // Try an example without a zero-denominator
+    a = rational<int>( -9, 12 );
+    BOOST_CHECK_EQUAL( a.numerator(), -3 );
+    BOOST_CHECK_EQUAL( a.denominator(), 4 );
+    BOOST_CHECK( a.denominator() > 0 );
+    BOOST_CHECK_EQUAL( gcd(a.numerator(), a.denominator()), 1 );
+    BOOST_CHECK_THROW( a.assign(-(INT_MIN + 1), INT_MIN), boost::bad_rational );
+    BOOST_CHECK( a.denominator() > 0 );
+    BOOST_CHECK_EQUAL( gcd(a.numerator(), a.denominator()), 1 );
+    BOOST_CHECK_EQUAL( a.numerator(), -3 );
+    BOOST_CHECK_EQUAL( a.denominator(), 4 );
+#endif
+}
+
 BOOST_AUTO_TEST_SUITE_END()