Boost logo

Boost :

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
    // 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;

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.


Boost list run by bdawes at, gregod at, cpdaniel at, john at