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
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
