std.rational -- update and progress towards review

[Joseph Rushton Wakeling]

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