Re: [boost] [Review] Formal Review: Boost.Ratio

Subject: Re: [boost] [Review] Formal Review: Boost.Ratio
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-10-05 17:10:40

Hi Roland,
----- Original Message ----- 
From: "Roland Bock" <rbock_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Tuesday, October 05, 2010 9:07 PM
Subject: Re: [boost] [Review] Formal Review: Boost.Ratio

>> - What is your evaluation of the implementation?
>>    
> The code looks very clean, except for a few #if 0, which I would rather 
> have removed.

I guess you are talking of these kind of #if 0

#if 0
public:
    // The nested typedef type  shall be a synonym for ratio<R1::num * R2::den, R2::num * R1::den>::type.
    typedef typename ratio<R1::num * R2::den, R1::den * R2::num>::type type;
#else

You are right that these kind of comments could be more troubling that helping.
What do you think about removing the #if 0 and letting the comment as follows?

// The nested typedef type  is a synonym for ratio<R1::num * R2::den, R2::num * R1::den>::type that avoids overflows when possible.

I can also remove it completly, as the documentation explain this problem widely.

>> - What is your evaluation of the documentation?
>>    
> Good to read, but needs consistency checks. For instance, the link
> 
> See the source file test/ratio_test.cpp
> 
> yields something completely different than the documentation...

Hrr. I changed the name of the file quite lately. I will update it on the documentation. I guess that I will need to run the inspection tool to catch other bad links.

>> - Did you try to use the library? With what compiler? Did you have any problems?
>>    
> As part of Chrono, yes.
> 
> gcc 4.2.4 on Ubuntu 8.04, 64bit
> gcc 4.4.1 on Ubuntu 10.4 64bit
> 
> No known problems.

I had no make test on 64 bits platforms. This is godd to know.
 
>> - How much effort did you put into your evaluation? A glance? A quick
>> - reading? In-depth study?
>>    
> When trying to fix warnings in Chrono (while ratio was still part of 
> Chrono), I spent several hours working through the code. For this 
> review, I looked at the header files rather briefly and read the 
> documentation.

Thanks for helping me fixing warnings and for the many suggestion you did concerning the documentation. 

>> And finally, every review should answer this question:
>>
>> - Do you think the library should be accepted as a Boost library?
>>    
> 
> Yes.

And of course thanks for your positive review.

Best,
Vicente