Hello all,
I thought I'd ask for some guidance here, since this is the first time I'm
involved in bringing a new Phobos module to review.
If I understand right, "review" means that I present a pull request to Phobos,
rather than just a standalone piece of code as currently available. The
question is how to handle various bits of std.rational that arguably could or
maybe should be implemented elsewhere in Phobos. Is there a preference to
present the module as-is, and have the review process decide what goes where, or
is it better to present the pull request including patches against other modules?
To help with answers to the above, I'll summarize the state of std.rational and
what I've done so far.
* std.rational is designed to allow the construction of rational numbers
based on any of the built-in integer types (it also works with char:-),
std.bigint.BigInt, or arbitrary user-defined "integer-like" types.
* Because it doesn't just want to work with built-in integer types, David
Simcha was forced to define custom versions of various functionality
found elsewhere in Phobos (or in some cases, perhaps at the time the
functionality simply was not there). Some of these have been deleted
in my recent updates as they are no longer necessary: e.g. today it is
fine to use std.math.abs, std.traits.isAssignable. Others remain, and
the question is how to handle these cases.
* David defines an isRational template, which is currently very simple
(it just checks that the type has numerator and denominator properties).
This presumably is fine to keep in std.rational and should not be moved
elsewhere.
* Because std.rational doesn't just want to work with built-in integer types
it can't rely on the existing isIntegral. Instead, David defined a new
template, "isIntegerLike", which checks the operations supported by the
type and whether they work in an integer-like way. Could/should this be
placed in std.traits? Better to do this in the initial pull or let the
decision be part of the review process?
* For similar reasons, CommonType is insufficient and David defined two
new templates, CommonInteger (which works out an appropriate common
type for two "integer-like" types) and CommonRational. The former at
least looks to me like it should be in std.traits. Again, better to
do this in the initial pull request, or make it a decision to take at
review time?
* Several Rational-based overrides for std.math functions are defined:
floor, ceil and round. I believe it's normal for such type-specific
overrides to be in the same module as their type?
* Finally, there are two mathematical functions, gcf (greatest common factor)
and lcm (least common multiple) which again are locally defined because
Phobos' existing std.math.gcd cannot handle BigInts (let alone any other
user-defined integer type). These should presumably be sent over to
std.math but that relies on isIntegerLike being accepted for std.traits,
or else some alternative type check being in place.
* (... actually, there is no lcm in std.math in any case. An oversight:-)
I need to do a careful review of the function attributes (there's a complete
lack of @safe, pure, nothrow, const, ... in the module, only @property is used),
but apart from that I think that the code is now working within the confines of
its design parameters, and in that sense is ready for review. I've squashed the
only overt bug found, added unittests to confirm this and to check other cases
(David's were already very extensive so I haven't needed to do much), updated
and corrected the docs and removed all unnecessary code duplication, and of
course also brought the code style in line with current Phobos standards.
The remaining open issues
<https://github.com/WebDrake/Rational/issues?state=open> are all design-related.
Apart from those raised by my above queries, the major one is how rationals
should relate to floating-point numbers -- e.g. there is currently no opCmp for
floating-point, meaning this:
assert(rational(10, 1) == 10);
... will work, but this:
assert(rational(10, 1) == 10.0);
... will fail to compile. It's not entirely obvious how to resolve this as
floating-point vs. rational comparisons risk accidentally creating huge
temporary BigInt-based rationals ... :-(
Anyway, I'd really value your feedback at this point.
Thanks & best wishes,
-- Joe
