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

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2010-10-06 11:10:04

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.

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.

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

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.

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.

It's odd to see enable_if_c without the namespace qualifier given the great number of times that "boost::" appears in the source.

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?

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.

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

There are a great number of names put into boost::detail that might conflict with those provided by other libraries.  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.

The helper code in the detail namespace would be better in a detail header to remove clutter from ratio.hpp.

Consider moving the empty function bodies to a separate line.  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.)

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

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.

s/Exceptions safety/Exception safety/

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?

Tested compilers: change to read, "Boost.Ratio should work with an C++03 conforming compiler.  The current version has...."

Tutorial, paragraph 1: boost::ratio cannot be used in "std-defined" libraries.

s/assignement/assignment/

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

s/double count/strain one's eyes carefully counting/

s/write million or billion/write millions or billions/

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

The Examples section is misnamed given there's but one example in it.

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.

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

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:

   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]/

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.

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

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

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

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

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.

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

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

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

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.

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
___________________________

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

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.

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.

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.

Appendix G: remove from the final documentation.

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

Very useful

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

No.

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

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.