Hi Roland, ----- Original Message ----- From: "Roland Bock" <rbock@eudoxos.de> To: <boost@lists.boost.org> 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