[phobos] Rational for review

[Andrei Alexandrescu]

This proposal looks almost ready for bringing up to digitalmars.d for a 
formal review. Essentially you'd need to cross all "t"s and dot all "i"s 
following suggestions from reviewers on this list, and work on creating 
a good quality documentation.

I have a few comments and suggestions based on what I just saw at 
http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d:

* I want us to cover in this same module the case of a rational with a 
fixed denominator, i.e. a fixed-point type. For example, a financial 
program might use FixedRational!(long, 100) to operate consistently on 
cents. (As a curiosity, in the olden days (up until 2002 or so) the 
stock market operated on FixedRational!(long, 8192).) There are plenty 
of places in which fixed point values are desirable, and I believe many 
of them can be expressed as rational values with a compile-time-known 
denominator.

* The import section should be ordered alphabetically.

* The test isRational suggests that you accept any values for num and 
denom, but in reality you should constrain that to e.g. isIntegerLike 
for both.

* isAssignable should be transferred to std.traits

* Isn't CommonType good in place of CommonInteger?

* Can't you consolidate the first two implementations of op"*"?

* Inside opOpAssign, you have the code:

auto divisor = gcf(this.numerator, rhs.denominator);
this.numerator /= divisor;
rhs.denominator /= divisor;

I have the suspicion that an organic routine that computes the gcf and 
simplifies at the same time could save on some divisions. Don't forget 
that integral divisions are very, very slow. Same considerations applies 
for other similar patterns in code.

* Is it worth using Unsigned!T for the denominator throughout? That way 
there's no more need to worry about fixSigns etc.

* When you swap() in op"/", don't forget to assert that the 
denominator-to-be is nonzero. Generally I'm not seeing asserts/contracts 
throughout.

* I have the suspicion that implementing e.g. += in terms of + is 
actually slower than creating the value directly. Generally on today's 
architectures you want to minimize mutation, and += is more mutation than +.

* In opCmp, I suspect that converting to double and carrying the 
computation may be actually cheaper.

* Anyhow, in opCmp instead of

if(lhsNum > rhsNum) {
   return 1;
} else if(lhsNum < rhsNum) {
   return -1;
}
// We've checked for equality already.  If we get to this point,
// there's clearly something wrong.
assert(0);

do this:

assert(lhsNum != rhsNum);
return lhsNum > rhsNum ? 1 : -1;

* Why the use of "this." in some places?

* In opCast!double, can't you use some specialized routines in BigInt 
instead of rolling your own? Regardless, I suspect BigInt should have a 
solution for "divide yielding a floating point number" - if not, you may 
want to discuss with Don moving your implementation into std.bigint.

Again, looking forward to see a formal proposal on digitalmars.d soon!


Andrei

On 8/27/10 7:48 PM, David Simcha wrote:
> I've cleaned up/fixed the bit rot in my Rational library. It's available
> for review at:
> http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d?rev=785
> .
>
> Known issues:
>
> 1. Due to bugs in BigInt (mainly 4742
> http://d.puremagic.com/issues/show_bug.cgi?id=4742), BigInt
> instantiations don't play nicely with builtin integer instantiations.
> For example, this doesn't work:
>
> auto a = rational(BigInt(1), 2) * rational(8, 7);
>
> 2. I couldn't figure out how to handle the case of user-defined
> fixed-width integer types in the decimal conversion (opCast) function,
> so it just assumes non-builtin integer types are arbitrary precision.
> The biggest problem is lack of a way of introspecting whether the
> integer is fixed or arbitrary width and what its fixed width might be if
> it is, in fact, fixed. The secondary problem is that I don't know of a
> good algorithm (though I'm sure I could find one if I tried) for
> converting fixed-width integers to floating point numbers in software.
> Arbitrary precision is much easier.
>
> 3. There is a small amount of special case ad-hockery to make things
> work with std.bigint.BigInt. I consider these bug workarounds. In
> principle, everything should work with any user defined arbitrary
> precision integer if it overloads everything it's supposed to overload.
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos