Re: [boost] [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY
From: Tan, Tom (Shanghai) (TTan_at_[hidden])
Date: 2010-10-10 23:24:06

Hi,
Here my review.
----- Original Message ----- 
From: "Anthony Williams" <anthony.ajw_at_[hidden]>
Subject: [boost] Reminder: [Review] Formal Review of Proposed
Boost.RatioLibrary IS UNDERWAY
>> Here are some questions you might want to answer in your review:
>>
>> - What is your evaluation of the design?
As others already mentioned, it follows the C++ Standards Committee's
current Working Paper. That's good.

>> - What is your evaluation of the implementation?
- Code is clear and easy to read and understood.
- BOOST_INTMAX_C is the only macro that's defined in ratio.hpp but not
staring with BOOST_RATIO_. As the name suggests, it'll be better 1) if
placed in boost/cstdint.hpp, or 2)be named as BOOST_RATIO_INTMAX_C
indicating that it's used for boost.ratio only, I personally prefer
option 1).
- I would expect the comments are better cleaned up and the style
unified.  Right now, //, //-, //~, /**/ are all used, which seems
confusing. I personally like the style adopted in boost.asio.

>> - What is your evaluation of the documentation?
- It's always a pleasure to read documents generated with BoostBook
Documentation Format,   
- In the tutorial section, it'll be great if more examples are appended
so that one example for each supported arithmetic and comparison
operations, considering boost.ratio is a small one, doing so won't
explode the tutorial section but gives a more complete picture of the
lib.

>> - What is your evaluation of the potential usefulness of the library?

I can think of two scenarios:
1) It could quite useful in physical measurement of time, length, weight
and the like where values of different units are being calculated. 
2) It could be useful in expressing large rational number
(Can anyone please let me know whether ratio is short for rational or a
complete word itself? It seems either is OK, though I still wonder)

>> - Did you try to use the library? With what compiler? Did you have
any problems?

Yes, indirectly through using boost.chrono, which is based on
boost.ratio.
VC10 on Windows 7, 
GCC4.4 w/ mingwin on widows 7.
GCC4.4 on Ubuntu 10.4
No problem with the latest version from boost sandbox. 

>> - How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I did have some in-depth reading of previous versions of the code &
documentation while I was started using boost.chrono, though not line by
line.
I also did a deep reading of C++ Standards Committee's current Working
Paper (n2661in order to understand the why and how of ratio class then.
While doing this review, I had a thorough reading of the code and
documentation, which costs about half a day (including expressing my
comments in English).

>> - Are you knowledgeable about the problem domain?

Intermediate.

>> - Do you think the library should be accepted as a Boost library?
Yes.