Re: [boost] [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY
11 Oct
2010
11 Oct
'10
3:24 a.m.
Hi, Here my review. ----- Original Message ----- From: "Anthony Williams" <anthony.ajw@gmail.com> 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.
11 Oct
11 Oct
8:44 p.m.
New subject: [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY
Tan, Tom (Shanghai) wrote: > > ----- Original Message ----- > From: "Anthony Williams" <anthony.ajw@gmail.com> > 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.
12 Oct
12 Oct
7:38 a.m.
New subject: [Review] Formal Review of Proposed Boost.RatioLibrary IS UNDERWAY
- 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.
The C99 macros INTMAX_C and UINTMAX_C are already present in boost/cstdint.hpp. HTH, John.
5357
Age (days ago)
5358
Last active (days ago)
2 comments
3 participants
participants (3)
-
John Maddock -
Tan, Tom (Shanghai) -
viboes