RE: rational.hpp

From: Moore, Paul (paul.moore_at_[hidden])
Date: 2001-01-12 05:51:13

From: "Stephen Silver" <boost_at_[hidden]>
> I've been trying out the Boost rational number library, and have come
> across some problems.  These problems are only encountered when using
> a user-defined type (in my case, an arbitrary-precision integer type)
> as the template argument.

Thanks for the feedback. I've long wanted to have rationals based on
arbitrary precision integers - that was my long-term goal as the "right"
thing. Unfortunately, I haven't had access to an arbitrary precision integer
type (nor the time/expertise to implement one). Any chance yours could be
submitted to Boost?

> The first problem is fairly minor.  The gcd() function template in
> rational.hpp is documented as working for "any types for which the
> following operations are defined: =, +=, *=, /=, %, <".  However, it
> contains the line
> 
>     while (m) {
> 
> so it also requires that implicit conversion to bool be 
> possible.  This is not a very desirable requirement, because user-
> defined conversion functions that convert to built-in types are
> not very safe (due to the possibility of the conversion happening
> in contexts where it was not anticipated).  Note that the above
> line could be changed to
> 
>     while (!!m) {
> 
> which would have the advantage that it could work by means of 
> operator! instead of requiring implicit conversion to bool.

Thanks for pointing this out. I won't use !! though (as someone pointed out
in a later post, the integer type may not have operator! either). My initial
preference is for "while (m != 0)" - would that cause a problem (I think it
requires that the integer type be comparable with built-in integers, hardly
an unreasonable requirement)?

> The second problem will arise with any user-defined integer 
> type that is used with boost::rational.  Suppose Integer is
> such a type, and we have:
> 
>     rational<Integer> r(2);
>     r += 2;
> 
> This simple operation will not compile, because it requires the
> application of two successive implicit user-defined conversions
> (int -> Integer -> rational<Integer>).  This means that it's
> necessary instead to write
> 
>     r += Integer(2);
> 
> This gets pretty cumbersome when you have to do it a lot.  And you
> usually do have to do it a lot, because it applies to just about every
> operator that boost::rational defines.  The problem can be cured by
> adding a template constructor like this:
> 
>     template <class T> rational(T n) : num(), den(1) { num = n; }

Good catch - I'll add this.

> (I've set the value of 'num' by assignment rather than initialization
> because initialization could make use of a constructor that had been
> declared explicit, which may not be desirable - although it's hard to
> think of a realistic situation in which it would cause a problem.)

Hmm. Overall, I agree with this, although I wonder whether there could be
cases where default-construct and assign is significantly slower than just
constructing. Seems far less likely than even the explicit constructor case,
though, and it's "only" performance, so I'd go with it.

Thinking some more, I suspect that we have to use this version as we must
avoid constructing a rational<T> from a type U when the only constructor
from U to T is explicit. So yes, I go with your code.

> Lastly, there is a question of efficiency.  The 'assign' 
> member function and two of the constructors take arguments
> of type IntType. For use with user-defined integer types
> it would be better to have arguments of type const IntType&.
> Perhaps the choice between IntType and const IntType& should
> be made to depend upon the type (using boost::is_standard_integral,
> for example).

Urk. This is all that fancy stuff that Howard and John have been having so
much fun with. Isn't there some sort of
boost::most_efficient_way_of_passing_a_parameter<T> that does what I want?
:-)

To be honest, the discussions round this lost me. I'd appreciate it if
someone could give me a pointer to what I need to do for this. And also tell
me if it works on MSVC (as I use MSVC, I'm unwilling to include something in
rational.hpp which breaks MSVC compatibility...)

Thanks again for the useful feedback.

(Must get an update done with all this new stuff...)
Paul.