[boost] Some comments regarding class rational

From: Jens Maurer (jmaurer_at_[hidden])
Date: 2000-02-10 10:04:08

Dear Paul,

I have now found the time to look a bit more closely at your rational
class.

Here are a few comments regarding your implementation:

 - I tend to order #include's in my source files so that standard
ones are first.  This would put <cstdlib> before <boost/operators.hpp>.
You may consider this nitpicking; feel free to ignore this item.

 - rational.hpp needs a #include <string>, because the bad_rational
class relies on the automatic type conversion from const char[] to
std::string.  The variant of the standard library I use here has only 
a declaration of std::string visible in <stdexcept>, not the full
definition. However, the latter is required for the non-explicit
one-argument constructor to work.

 - The rational<> class has the invariant that its denominator can
never be 0. However, the implementation tries to enforce this at the
wrong places. Let's check all places where it could possibly happen:
   - the two-argument constructor is fine, normalize() throws, and
     the rational object never exists.
   - assign() is wrong, it assigns first and then calls normalize() to
     throw the exception. However, the damage to the previous state of
     the object has already been done.
   - operators +=, -=, *=, ++, --, <, ==, +, - can never produce a
     denominator of zero if the input is ok.
   - As someone else already pointed out, operator/= does not throw the
     exception.
Note that operators += and -= call normalize() which checks for a
denominator of zero although it would not be necessary in these cases.
I suggest removing the check for a zero denominator from normalize()
and instead add it to the two-argument constructor, assign() and
operator/=.

 - operator-=() has the normalize() call in the wrong place
(compare with operator+=()). Thus, 5/6 - 2/15 returns 7/30 instead
of the correct 7/10.

 - Both gcc 2.95.2 and a recent CVS checkout of the gcc development
version give me the error
"undefined reference to `abs(boost::rational<int> const &)" when
(compiling and) linking rational_example.cpp.
Reading section 14.5.3 [temp.friend] of the standard, it is clear 
that the friend declaration itself does not trigger automatic function
template instantiation.  However, the later template function
definition should.  It's probably a gcc bug.

 - I cannot see why rational_example.cpp needs #include <exception>

Jens Maurer.