std.rational -- update and progress towards review

[Jesse Phillips]

On Wednesday, 2 October 2013 at 14:26:04 UTC, Joseph Rushton 
Wakeling wrote:
> 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.

You've gotten some general input, I'll provide mine.

> 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?

People want to be able to review how the code will fit into 
Phobos, having the source incorporated in a branch of Phobos is 
the best way (and makes it easier to generate docs which resemble 
Phobos). A pull request should not be created.


>    * 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.

If the custom changes can be made to work with the existing 
functions, it can replace those existing functions. E.g. 
std.math.gcd can be updated to accept Rational as long as it 
continues to work on all types.

>    * 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.

Yes, leave the trait check inside the rational library. We don't 
have an integer... library so std.traits contains a number of 
traits for built in types.

>    * 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?

Add to std.traits for below.

>    * 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?

Keep private for now.

>    * 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?

Yes, if it is just an override leave it here.

>    * 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.

Fix std.math to work with the new type.

You'll want to get the public interface as polished as possible. 
Another solution would be to remove gcf/lcm from public API, if 
missing those would hold back inclusion then people will tell 
you. Once the module has been accepted then pull requests can be 
done to fix gcd and add lcm and it will not need formal review.