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.
