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

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY
From: viboes (vicente.botet_at_[hidden])
Date: 2010-10-11 16:44:50

Tan, Tom (Shanghai) wrote:
> 
> ----- 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 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'll see if we can add it to cstdint.hpp. Waiting for that I will rename the
macro.

> - 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.
> 

I don't see any major problem with that, but I'll try to be more uniform.

>>> - What is your evaluation of the documentation?
> - 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.
> 

You are right, these are no documented on the tutorial. Even if its use is
simple, I will try to find an example that show the utility.

> (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)
> 

Ratio comes from Latin and means proportion (the word exists in some
language like English or French as such). Rational is a number.

>> - 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).

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

Thanks Tom,
Vicente

-- 
View this message in context: http://boost.2283326.n4.nabble.com/Re-Review-Formal-Review-of-Proposed-Boost-RatioLibrary-IS-UNDERWAY-tp2983933p2990595.html
Sent from the Boost - Dev mailing list archive at Nabble.com.