Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-10-07 01:56:40

Hi,

thanks for your constructive review.

----- Original Message ----- 
From: "Stewart, Robert" <Robert.Stewart_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, October 06, 2010 5:10 PM
Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library

> 
> Anthony Williams wrote:
>>
>> The review of Vicente Botet' Ratio library starts TODAY (October 2nd)
>> and lasts until October 11th, 2010, unless an extension occurs.
> [snip]
>> - What is your evaluation of the design?
> 
> There's only so much wiggle room relative to the standard.  It appears that the relevant Boost constructs are used where appropriate, so it's as close as it can get without being std::ratio.  However, I wonder if this library should include the means to fall back on std::ratio when available.

I think that we could have the equivalent of the TR1 library for C++0x. I'm not wrong the user using the Boost.TR1 is requesting for the standard when available and use as fall back the Boost implementation. So if we do the same way, we will need a Boost.C++0x that will include not only ratio, but all TR1, Boost.Thread, ...

I'm voluntering to participate in this if the Boost community think it is the way to manage with C++0x libraries.

> The use of enable_if to control the contexts in which the copy constructor and copy assignment operator apply goes beyond the standard's specification.  That means boost::ratio behaves differently than will std::ratio.  I think this will lead to surprising results when one transitions from one to the other.

I 've made a request to add these constructors and this request has not ben decided yet. I thinh the best for Bost could be a flagf that intruduce this feature or not. So i prupose that Boost.Ration includes thes constructors and assignemes conditionally.

>> - What is your evaluation of the implementation?
> 
> Some macros have a redundant "RATIO_," such as BOOST_RATIO_RATIO_NUMERATOR_IS_OUT_OF_RANGE.  Why isn't that BOOST_RATIO_NUMERATOR_IS_OUT_OF_RANGE?

The reason is that every macro is prefixed by BOOST_RATIO. If the message is related to the ratio numerator the message is RATIO_ENUMERATOR ... if it is relaeted to the ratio adistion by BOSTST_RATIO_RATIO_ADD. 

> The #if 0/#endif blocks should include section/paragraph citations to the (draft) standard and should be made ordinary comments.  As it is, such code can be syntax highlighted and look active.

Yes. As said in another post, I will transform these conditional complilations by commets.
 
> Code commented out, like in lines 295-296 should be removed or commented to explain its presence.  That is, if the code is to illustrate a point, then there should be a comment explaining its presence.

You are right this comments should be removed.
 
> boost::intmax_t is used much of the time, but not always.  It is possible for intmax_t and boost::intmax_t to refer to different types.

This could be possible, I would be interested in seen a concrete case in te library code.
 
> It's odd to see enable_if_c without the namespace qualifier given the great number of times that "boost::" appears in the source.

I would say that any boost::qualilfied symbol must be explicitly explained.

> There is no bow to C++0x support.  For example, ratio<>::num and den are specified as static constexpr intmax_t in the standard.  Shouldn't they be declared the same in Boost.Ratio when the compiler supports it?

The library will use compilers support for constexpr. UNfortunately I have no access to them up to now.

> I'm pleased to see that the specified constraints on N and D are asserted at compile time to prevent misuse with helpful diagnostics.
> 
> If the enable_if gymnastics are removed, then the default constructor is no longer needed.

Could you be more specific?
 
> If m_na and m_da were renamed to, say, abs_N and abs_D, and were defined earlier, then the range checks could reuse them for greater clarity:
> 
>   static const boost::intmax_t abs_N = boost::detail::static_abs<N>::value;
>   static const boost::intmax_t abs_D = boost::detail::static_abs<D>::value;
>   BOOST_RATIO_STATIC_ASSERT(abs_N >= 0, BOOST_RATIO_NUMERATOR_IS_OUT_OF_RANGE, ());
>   BOOST_RATIO_STATIC_ASSERT(abs_D > 0, BOOST_RATIO_DENOMINATOR_IS_OUT_OF_RANGE, ());
> 
Yes this could be a detail implementation improvement, but not too important to me.

>There are a great number of names put into boost::detail that might conflict with those provided by other libraries.  

Please, could you give some examples?

> Indeed, many of those names are for useful components that could be part of Boost.Utility if they aren't moved into a ratio-specific detail directory.  I'm referring to, for example, static_abs, static_sign, and static_gcd.

For static_gcd I'm not using the existing math staic_gcd as the tipe parameters are not the same and Rato needs the signed type.
 
> The helper code in the detail namespace would be better in a detail header to remove clutter from ratio.hpp.

I have been the mentor of a Soc library that pretends to add all theses stuf and more. I hope that hese willbe included in Boost one day, but I dont think the detail implementations should block the library acceptation.
I could moce these to a file ina the detail directory if you prefer.
 
> Consider moving the empty function bodies to a separate line.  

I'm a little lsot. Please, could you be more explicit?

> Doing so eases debugging in many cases.  I don't expect much occasion to put breakpoints in ratio's functions, but it can happen.
> 
> According to 20.6.2, ratio_add<R1,R2> is to be a synonym for ratio<T1,T2> where.... Indeed, it is written as template <class R1, class R2> using ratio_add = ....  Thus, ratio_add is a typedef name for ratio.  What I find implemented is that ratio_add<R1,R2>::type is ratio<T1,T2> when I expected to find that ratio_add<R1,R2> would derive from ratio<T1,T2> (for C++03).  Did I miss something?  (The same applies to the other arithmetic types, of course.)

I dont't see wehre you see the problem. Could you clarify?
 
>> - What is your evaluation of the documentation?
> 
> Overall, very good.  Here are some comments:
> 
> Description, bullet 2: change to, "...std::basic_string, which can be useful for I/O."
> 
> s/Users'Guide/User's Guide/
> 
> The Getting Started section should be eliminated, and Installing Ratio should be promoted in its place, as there is just the one subsection.  Installing Ratio, in its final form, should simply state that, "Boost.Ratio is a header only library, so there is nothing to compile and it is ready to use as part of a normal Boost installation."

The fact there is not too must information in these section is not a reason to compress it. I think just that it must contains something more in the ...  

> Add a Design Notes section, or something of that sort, to contain the exception safety, thread safety, and compiler information presently in the Installing Ratio section.

I've followed the schema of the Boost.Proto library. So if we need to change something ....

> s/Exceptions safety/Exception safety/

OK.
 
> Reword that section to, "All functions in the library are exception-neutral, providing the strong exception safety guarantee."  To what "underlying parameters" were you referring?  Everything is compile time.
>
> Thread safety: What is thread unsafe about the compile time code?

This was intialy in the Boost.Chrono library which includes not only compile-time code. Yes, these two needs to be reworded.
 
> Tested compilers: change to read, "Boost.Ratio should work with an C++03 conforming compiler.  The current version has...."

OK.
 
> Tutorial, paragraph 1: boost::ratio cannot be used in "std-defined" libraries.
> 
> s/assignement/assignment/

OK.
 
> Tutorial, paragraph 2: documenting the public members as "static const intmax_t" precludes the C++0x-specified declarators: static constexpr intmax_t.

I think we can do this abuse of languagen but I could replace by static constant.
  
> s/double count/strain one's eyes carefully counting/
OK if this is clearer.
 
> s/write million or billion/write millions or billions/
OK.
 
> Tutorial, Example: use BOOST_STATIC_ASSERTs to illustrates what's presently in the comments:

> 
>   typedef ratio_multiply<ratio<5>, giga>::type _5giga;
>   BOOST_STATIC_ASSERT(_5giga::num == BOOST_INTMAX_C(5000000000));
>   BOOST_STATIC_ASSERT(_5giga::den == 1);

Why not?
 
> The Examples section is misnamed given there's but one example in it.

I expected to have more than one :(
 
> s/SI-units/SI units/
> 
> SI-units, paragraph 1: change to, "This example illustrates the use of type-safe physics code interoperating with boost::chrono::duration types, taking advantage of the Boost.Ratio infrastructure and design philosophy."
> 
> SI-units, paragraph 2: change to, "Let's start by defining a length class template that mimics boost::chrono::duration, which represents a time duration in various units, but restricts the representation to double and uses Boost.Ratio for length unit conversions:"
>
> SI-units, paragraph 3: change to, "Here's a small sampling of length units:"
>
> Note that code sample has an odd-looking extra space between "boost::" and "centi" and "kilo."  Something similar happens with "boost::chrono::" and "duration" later.

I see it and why. I should be able to remove this extra space.
 
> SI-units, paragraph 4: change to, "Note that since length's template parameter is actually a generic ratio type, so we can use boost::ratio allowing for more complex length units:"
> 
> SI-units, paragraph 5, change to, "Now we need a floating point-based definition of seconds:"
> 
> SI-units, paragraph 6, change to, "We can even support sub-nanosecond durations:"
> 
> SI-units, paragraph 7, change to, "Finally, we can write a proof-of-concept of an SI units library, hard-wired for meters and floating point seconds, though it will accept other units:"
> 
> SI-units, paragraph 8, change to, "That allows us to create some useful SI-based unit types:"
> 
> SI-units, paragraph 9, change to, "To make quantity useful, we need to be able to do arithmetic:"
> 
> SI-units, paragraph 10, change to, "With all of the foregoing scaffolding, we can now write an exemplar of a type-safe physics function:"
> 
> SI-units, paragraph 11, change to, "Finally, we can exercise what we've created, even using custom time durations (User1::seconds) as well as Boost time durations (boost::hours).  The input can be in arbitrary, though type-safe, units, the output is always in SI units.  (A complete Units library would support other units, of course.)"

Yes I will  take in account all your improving suggestions.
 
> External Resources: This section title uses title case, whereas most others do not.  I prefer title case, but you need to be consistent.
> 
> The Working Paper reference is outdated.  There should be a link to <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3126.pdf> and the section numbers need to be updated:

I've tried to avoid this direct reference.
 
>   s/20.4 Compile-time rational arithmetic "ratio"/20.6 Compile-time rational arithmetic [ratio]/
>   s/20.9 Time utilities "time"/20.11 Time utilities [time]/
>   s/20.6.7 Other transformations "meta.trans.other"/20.7.6.6 Other transformations [meta.trans.other]/

I will take the first, and remove the two others.

> Reference, Configuration macros: The paragraph ends with "Define" but each bullet contains "define it if."  I suggest this structure:
> 
> ___________________________
> When BOOST_NO_STATIC_ASSERT is defined, one can select the way static assertions are reported.  Define
> 
>    * BOOST_RATIO_USES_STATIC_ASSERT to use Boost.StaticAssert
>    * BOOST_RATIO_USES_MPL_ASSERT to use Boost.MPL static assertions
>    * BOOST_RATIO_USES_ARRAY_ASSERT to use Boost.Ratio static assertions
> 
> The default behavior is as if BOOST_RATIO_USES_ARRAY_ASSERT is defined.
> ___________________________
> 
> Note that I s/asertions/assertions/ and made other alterations.

OK.
 
> BOOST_RATIO_USES_ARRAY_ASSERT is an odd name.  Why not BOOST_RATIO_USES_RATIO_ASSERT or BOOST_RATIO_USES_INTERNAL_ASSERT?

I will take  BOOST_RATIO_USES_INTERNAL_ASSERT.

> s/following symbols are defined as/following symbols are defined as shown:/

OK 

> Reference, Configuration macros, last paragraph: change to read, "Depending upon the static assertion system used, a hint as to the failing assertion will appear in some form in the compiler diagnostic output."

OK 

> Reference, Class Template ratio<>, para 2: change to read, "The members num and den will be normalized values of the template arguments N and D computed as follows.  Let gcd denote...absolute value.  Then:"
> 
> Reference, Class Template ratio<>, para 3: change to read, "The nested...used when the normalized for of the template arguments are required, since the arguments are not necessarily normalized."

OK.
 
> Reference, Limitations and Extensions: there are many errors in this section, so here's a suggested rewrite for it all:
> 
> ___________________________
> The following are limitations of Boost.Ratio relative to the specification in the C++0x draft standard:
> 
>   * Four of the SI units typedefs -- yocto, zepto, zetta, and yotta -- are to be conditionally supported, if the range of intmax_t allows, but are not supported by Boost.Ratio.
>   * Ratio values should be of type static constexpr intmax_t, but no compiler supports constexpr today, so Boost.Ratio uses static const intmax_t instead.
>   * Rational arithmetic should use template aliases, but those are not available in C++03, so a nested typedef type is used instead.
> 
> The current implementation extends the requirements of the C++0x draft standard by making the copy constructor and copy assignment operator have the same normalized form.
> ___________________________
> 
> On the last point, since there was no consensus to add that behavior, should Boost.Ratio really behave differently than std::ratio will?  That will make transitioning to std::ratio harder.

I will remove them. I thougth that the point was not closed already. 

> Reference, Header <boost/ratio_io.hpp>, para 1: change to read, "This head provides ratio_string<> which can generate a textual representation of a ratio<> in the form of a std::basic_string<>.  These strings can be useful for I/O."

OK.
 
> Reference, Header <boost/ratio_io.hpp>, para 2: remove "Porting to Boost has been trivial."

OK.
 
> Reference, Header <boost/ratio_io.hpp>, para 3:
>   s/short_name/short_name()/
>   s/long_name/long_name()/
>   s/ratio's/ratios/ (though keep the code font on "ratio")
OK. 
> Much of the information in this section should be part of the non-reference half of the documentation.  That is, there should be a section in Tutorial on ratio_io.

I will do it.

> Reference, Header <boost/ratio_io.hpp>, paras 4&5: change these to the following:
> 
> ___________________________
> ratio_string<ratio<N,D>,CharT> is only defined for the following four characters types, with the indicated encoding of the strings:
> 
>   * char: UTF-8
>   * char16_t: UTF-16
>   * char32_t: UTF-32
>   * wchar_t: UTF-16 (if wchar_t is 16 bits) or UTF-32
> ___________________________
OK. 
> Reference, Header <boost/ratio_io.hpp>, Examples: these examples should be part of the User's Guide/Examples section.
> 
> Appendix A: remove from final documentation

Yes. I intend to start from zero.

> Appendix B: s/substract/subtract/
> 
> Appendix B, Why ratio needs CopyConstruction and Assignment from ratios having the same normalized form: this section should probably be removed unless it is thought good to keep this change and to push for a different resolution of LWG issue 1281.

OK.
 
> Appendix B, Why ratio needs the nested normalizer typedef type, change to read: "The current resolution of issue LWG 1281 acknowledges the need for a nested type typedef, so Boost.Ratio is tracking the likely final version of std::ratio."  There's no need for the rest of the text as issue LWG 1281 covers the details.

OK.
 
> Appendix C, How Boost.Ratio manage with compile-time rational arithmetic overflow?, para 2, bullet 3: change to read, "addition or subtraction that can...."
> 
> Appendix C, How Boost.Ratio manage with compile-time rational arithmetic overflow?, para 3: change to read, "The following subsections cover each case in more detail."
> 
> Appendix F: remove from the final documentation.
Yes; See my response in another POST. 
> Appendix G: remove from the final documentation.
Yes; See my response in another POST. 
 
>> - How much effort did you put into your evaluation? A glance? A quick
>>   reading? In-depth study?
> 
> I spent a couple of hours going though the code and documentation.
> 
>> - Are you knowledgeable about the problem domain?
> 
> Ummm, well, it isn't much of a domain.  I've a lot of experience in generic programming, etc., and I understand the (trivial) mathematics, so yes, I'm knowledgeable.
> 
>> - Do you think the library should be accepted as a Boost library?
> 
> Definitely.

Thank you for helping to improve the library.

Best,
Vicente