On Friday, 27 September 2013 at 10:11:47 UTC, Joseph Rushton Wakeling wrote:
> On 27/09/13 09:36, Walter Bright wrote:
>> For things to happen, usually a self-appointed champion is needed that will
>> relentless push it forward. Things tend to fall by the wayside otherwise.
>
> In the past David has seemed like a pretty reliable champion of his own work -- quite a few of the parts of Phobos I use regularly are down to him!
>
> I've written to him asking for his input on std.rational but if he doesn't have time/inclination to pursue it, I'll take a look at it.
>
> At first glance what's there looks pretty good with the main issues being stylistic rather than content-related.

As usual, volunteering to act as review manager. Once you are ready to championship this module, add it to review queue in wiki and ping me (preferably via e-mail)

On 27/09/13 12:37, Dicebot wrote:
> As usual, volunteering to act as review manager. Once you are ready to
> championship this module, add it to review queue in wiki and ping me (preferably
> via e-mail)

Thanks very much! :-)

I've made a fork <https://github.com/WebDrake/Rational> and tweaked the code to be in line with current D/Phobos style (David Simcha's last update is from over 2 years ago so it's out of date in that respect).  I did this manually, line by line, so I could have a proper read-through of the code.

On the basis of that read-through it looks pretty comprehensive and well-thought-out.  There are various workarounds for bugs which probably need to be re-examined depending on whether those bugs have been fixed or not.  In any case it compiles fine with current DMD, and the unittests are thorough and have 94% code coverage -- good job, David!

One thing I don't like: David uses "num" and "denom" for the public methods to get numerator and denominator.  I think it should be "numerator" and "denominator".  This matches Boost.Rational but I don't know if David was naming these methods by analogy to some other library/language.  In any case I should probably make a detailed comparison to Boost.Rational; I don't know if this was David's model or whether this is a clean-room implementation.

There are also probably some Ddoc tweaks that need to be made, not everything seems to be perfectly in order on my system.

Anyway, if anyone else wants to take a look through and give me any comments, they'd be welcome.  (Bearophile, I will add your requested Fraction, but I'd like to get more info on what everyone else thinks of the code as it is before making any additions:-)

Best wishes,

    -- Joe

On Fri, Sep 27, 2013 at 03:47:20PM +0200, Joseph Rushton Wakeling wrote: [...]
> I've made a fork <https://github.com/WebDrake/Rational> and tweaked the code to be in line with current D/Phobos style (David Simcha's last update is from over 2 years ago so it's out of date in that respect).  I did this manually, line by line, so I could have a proper read-through of the code.
> 
> On the basis of that read-through it looks pretty comprehensive and well-thought-out.  There are various workarounds for bugs which probably need to be re-examined depending on whether those bugs have been fixed or not.  In any case it compiles fine with current DMD, and the unittests are thorough and have 94% code coverage -- good job, David!
> 
> One thing I don't like: David uses "num" and "denom" for the public methods to get numerator and denominator.

For the record, I'm OK with it. I don't like "numerator" and "denominator" as it's needlessly verbose. It's already clear what "num" and "denom" mean in a rational number library, no need to spell it all out.

But, others may have a different opinion on this. As far as the review is concerned, I don't mind either way.


[...]
> Anyway, if anyone else wants to take a look through and give me any comments, they'd be welcome.

I'll see if I can take a look at it sometime -- no guarantees, though, as I'm busy with other things these days.


> (Bearophile, I will add your requested Fraction, but I'd like to get more info on what everyone else thinks of the code as it is before making any additions:-)
[...]

Honestly, I don't think that's necessary. It's just a one-line alias, trivial to add in user code. For Phobos code, I think writing Rational!BigInt is better, to make it clear what underlying number type is being used to instantiate the template.


T

-- 
One Word to write them all, One Access to find them, One Excel to count them all, And thus to Windows bind them. -- Mike Champion

27-Sep-2013 17:47, Joseph Rushton Wakeling пишет:
> On 27/09/13 12:37, Dicebot wrote:
[snip]
> One thing I don't like: David uses "num" and "denom" for the public
> methods to get numerator and denominator.  I think it should be
> "numerator" and "denominator".  This matches Boost.Rational but I don't
> know if David was naming these methods by analogy to some other
> library/language.

I bet the reason is practicality: try using full names of
denominator/numerator in some involved numeric code. It's a mess.
One may argue you need not access numerator and denominator explicitly that much but I think it happens.

-- 
Dmitry Olshansky

On 9/27/2013 6:47 AM, Joseph Rushton Wakeling wrote:
> [...]

Sounds like we have a new champion! Thanks, Joseph!

On Friday, 27 September 2013 at 15:36:12 UTC, Dmitry Olshansky wrote:
>
> I bet the reason is practicality: try using full names of
> denominator/numerator in some involved numeric code. It's a mess.
> One may argue you need not access numerator and denominator explicitly that much but I think it happens.

I'm not sure if this fits with Phobos style, but they could be defined as "numerator" and "denominator" and aliased to "num" and "denom", respectively. That way, we get the best of both worlds.

On 9/27/13 1:08 PM, BLM768 wrote:
> On Friday, 27 September 2013 at 15:36:12 UTC, Dmitry Olshansky wrote:
>>
>> I bet the reason is practicality: try using full names of
>> denominator/numerator in some involved numeric code. It's a mess.
>> One may argue you need not access numerator and denominator explicitly
>> that much but I think it happens.
>
> I'm not sure if this fits with Phobos style, but they could be defined
> as "numerator" and "denominator" and aliased to "num" and "denom",
> respectively. That way, we get the best of both worlds.

No aliases please. num and dom is where it's at.

Andrei

On 9/27/2013 1:25 PM, Andrei Alexandrescu wrote:
> No aliases please.

I agree. Aliases are not a solution to bikeshedding about names and inability to reach a consensus.

On 27/09/13 20:20, Walter Bright wrote:
> On 9/27/2013 6:47 AM, Joseph Rushton Wakeling wrote:
>> [...]
>
> Sounds like we have a new champion! Thanks, Joseph!

I feel more like a cheerleader than a champion -- David did all the hard work! ;-)

On 27/09/13 17:36, Dmitry Olshansky wrote:
> I bet the reason is practicality: try using full names of
> denominator/numerator in some involved numeric code. It's a mess.

Well, obviously.  Actually I'm not unhappy with David's name per se but with the idea of being inconsistent with how other libraries name these methods. Boost.Rational uses the full names, I'll have a look around and see how other libraries name them and whether there is consensus.

Next week I'll try and do a close comparison of Boost.Rational and David's work to see if there's any overlap, at least in the core data type.

Is there any expectation that some later iteration of the C++ is going to specify a rational type?  If so, any expected naming convention?  I guess Boost reflects what would be implemented if so, but it's never an absolute, things in Boost.Random were dropped in the C++11 standard.

> One may argue you need not access numerator and denominator explicitly that much
> but I think it happens.

I agree.