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);
> 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
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
> 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
> 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
> 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);
> 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
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
> 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>.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk