[boost] Re: Submission: Rational numbers

From: Moore, Paul (Paul.Moore_at_[hidden])
Date: 1999-12-10 10:43:39

From: Ed Brey [mailto:brey_at_[hidden]]
> 
> First, one interface issue:
> 
> Should there be an assign function, similar to what std::string has. 
> This would allow you to reassign a rational without creating a
> temporary.  For example, in the read from stream operator, code would
> change from
> 	r = rational<Int>(n, d);
> to
> 	r.assign(n, d);
> 

Hmm, I see your point. It could certainly be added. But I don't think
std::complex has this (that was my model). I don't mind one way or the
other.

OK, I'll add one. Should it return a reference to the assigned rational?
That allows things like r.assign(n,d).mem_fn(), but as rational has no
non-trivial members, this may be overkill. I'll return the ref, to match
string.
 
> Seconds, several issues with the code:
> 
> 1. Int lcm(Int n, Int m)
>    {
>       return (n * m) / gcd<Int>(n, m);
>    }
> 
> The <Int> for gcd seems unnecessary, since it is deducible from n and
> m.

Reasonable point.

> 2. The destructor defined in bad_rational is superfluous.  As an
> example, std::domain_error defines no destructor.

Hm. My implementation (Visual C++, yes I know) does have a destructor. I
have vague recollections that defining an empty virtual destructor can be
important in some cases, but I can't recall from where. I just copied the
library code. If it isn't needed it can certainly go. Anyone else have any
comments?

> 3. Of the base classes of rational, only less_than_comparable is
> public; all others are private.

Oops. How on earth did I miss that?!?

> 4. "normalise" is misspelled, should be "normalize".

UK vs US spelling. I prefer the UK version...

> 5. You comment that default assignment is fine, but say nothing about
> default copy constructor being fine.  I'd recommend dropping the
> commented out operator= line and changing the comment to "Default copy
> constructor and assignment are fine".

Yes. It was just a note to myself that I didn't keep up to date.

> 6. For indenting, a combination of spaces and tabs are used, which
> assumes a tab size of 8 spaces.  I would prefer using just spaces or
> just tabs, as the current representation causes problems if the tab
> size is set to other than 8.

No problem, I'll use all spaces.
 
> 7.  bool operator== (const rational& r) const
>     {
> 	return ((num == r.num) && (den = r.den));
>     }
> 
> = should be ==.

Aargh! I'm glad someone is checking this garbage!!!! :-(
 
> 8.  const rational operator++(int)
>     {
> 	rational was(*this);
> 	operator++();
> 	return was;
>     }
> 
> Clever variable naming in the use of "was".  I really like it.
> BTW, the postfix operators tend to always be boring code (except for
> creative variable names) that ends up just making a temporary and
> calling the prefix version.  Is there any way to add this to
> operators.hpp?

I don't recall who I got the idea from. But I agree, it's memorable. And
yes, it is dull boilerplate.

> 9. abs, operator+/- (unary), and rational_cast take parameters by
> value, where as all others take parameters by reference.  Why?

Ummm. I don't recall if I had a reason. I'll change them all to const
reference.
 
> 10. In noticed that abs ended up being a friend, even though the last
> post I saw led me to believe that it wouldn't be.  As I mentioned
> earlier, my understanding is that it should be a friend, so that it
> can access private data, if it should need to.  To me, it seems that
> it should have equal access rights as, say operator*= of any other
> tightly coupled function.  I'd be interested in hearing reasons for
> the belief that abs should not be a friend (discounting broken
> compiler work-arounds).

Basically, there is no *need* (in the presence of numerator() and
denominator()) for it to be a friend. I have no strong opinions one way or
the other as to the rights and wrongs of it being a friend - you have to
draw the line somewhere. But as you were recommending making it a friend,
and no-one said that was wrong, I decided to make it a friend. But it's not
a big deal, as far as I can see.

> 11. As the comment for abs states, abs calls std::abs on the
> denominator, even though it doesn't need to.  The compiler, will
> rarely know that this call is unnecessary.  Rather than take the
> performance hit, it would be better to take the std::abs off the
> denominator, and move the comment about abs's assumption up to the
> beginning of the document where someone who would change whether the
> function is normalized will see it.  Even more secure, you could
> create a normalized trait and do a compile-time check, although for a
> class this small, it doesn't seem worth it; however, a normalized
> trait may be useful for other reasons.

OK. I killed the extra abs() and put a comment alongside the declaration of
normalise() to the effect that fractions are held normalised, and that there
is an implication in abs().

I still believe that for a class like this, picking a representation and
sticking to it is the right choice. Witness the std::complex class, which
doesn't allow a choice between polar and cartesian representations. Giving
the user too much choice just makes the interface more complex.
 
> 12. You include <cstdlib> twice.  Also, there is an blank line break
> between inclusion of operators and config.

Yes. I added the "put abs into std::" hack from boost/config.hpp, and missed
that I already had cstdlib there.

Thanks for the helpful comments.

I attach a revised <rational.hpp>.

Paul.