Hi, Sorry for the late reply, yesterday was public holiday so I had a small vacations. Seg, 2010-10-04 às 22:52 +0200, vicente.botet escreveu:

Hi,

first of all, thanks for your positive review.

----- Original Message ----- From: "Bruno Santos" <bsantos@av.it.pt> To: <boost@lists.boost.org> Sent: Monday, October 04, 2010 10:15 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library Starts TODAY

...

...

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

Very useful on my domain, which is network protocols. I have to deal with all sorts of time (and other) units. And of course, Boost.Chrono needs it :-)

I'm interested in examples you could have. This can be done by private mail if you prefer.

Sure, I can give you some examples. I'm bit busy, but by the Weekend I can post some examples. Send me a reminder if I forget.

...

...

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

Yes. gcc, msvc and clang. No.

gcc-4.4 and latest clang-2.9svn on Ubuntu x86-64. msvc-10 x86 and x86-64 on Win7 x86-64, I not so sure about this last one because only today I have replace the RC with the RTM version. I will give a spin of test again to confirm this.

Glad to hear you don't find any issues :) Please could share with us with wich compiler version and on which platforms have you used the library?

...

...

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

Yes.

Thanks again, Vicente _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Hi, thanks for your constructive review. ----- Original Message ----- From: "Stewart, Robert" <Robert.Stewart@sig.com> To: <boost@lists.boost.org> 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

vicente.botet wrote:

From: "Stewart, Robert" <Robert.Stewart@sig.com>

...

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.

As I read the LGW issue, there was no support for adding that behavior.

...

...

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

I figured as much, but the names are clear without the redundancy. To what does "numerator" apply if not to the ratio? You should shorten the names when clarity isn't sacrificed. That means that BOOST_RATIO_RATIO_ADD might be acceptable, though I don't find such an example in ratio.hpp. Indeed, the three names I find would be quite clear without the redundancy.

...

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.

Here's a list of lines which refer to intmax_t without the "boost::" scope: 293 324 332 475 476 482 488 494 500 501 508 509 Note that all of those lines appear within the boost namespace, so while they may well refer to boost::intmax_t, they could cause an ambiguity with std::intmax_t if a using declaration or directive pulled it into the global namespace.

...

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 the final determination of LWG 1281 is to not support your suggestion, then you would presumably remove the copy constructor and copy assignment operator at which point the default constructor would no longer be 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, ());

Yes this could be a detail implementation improvement, but not too important to me.

I was referring to what would appear when the user violates the assertions. I wasn't concerned about the names you chose for your implementation.

...

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?

I did in what you quoted next. Your names appear in boost::detail, so if another library did the same, including both headers could cause conflicts.

...

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.

Perhaps the math version should be expanded?

...

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.

They clearly didn't affect my acceptance.

I could moce these to a file ina the detail directory if you prefer.

I was just thinking of folks who wander into ratio.hpp. The code would be clearer with less of the implementation details at first. Making that change is purely aesthetic, however, and you should do it only if you think the result worthwhile. What I asserted was merely opinion and I didn't mean for you to take it otherwise.

...

Consider moving the empty function bodies to a separate line.

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

Search for "{}" and move them to their own 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.)

I dont't see wehre you see the problem. Could you clarify?

As I understand it, one can use std::ratio_add<R1,R2> in a context requiring a std::ratio<T1,T2>. To do something similar with Boost.Ratio means using boost::ratio<R1,R2>::type.

...

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

Go for it.

...

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

If you say so. I don't find that sort of information in Proto's docs, much less section headings such as yours.

...

s/double count/strain one's eyes carefully counting/ OK if this is clearer.

I didn't understand "double count" at first, and the alternative is more evocative.

...

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?

Are you asking why not keep the comments or saying my suggestion is an acceptable alternative that you're willing to adopt?

...

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.

OK. The section references were what I meant to indicate by my use of "outdated." Bad editing on my part. _____ Rob Stewart robert.stewart@sig.com 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.

Howard Hinnant wrote:

...

...

From: "Stewart, Robert" <Robert.Stewart@sig.com>

...

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. [snip] I would be tempted to consider this extension to the standard semantics based on its technical merits alone, motivated by Beman's more general policy statement:

On Oct 2, 2010, at 8:11 AM, Beman Dawes wrote:

...

One of the advantages of Boost is that we can add extensions and get user experience before something gets standardized. That's very helpful to the C++ committee.

That's a fine approach, but it should be treated as an extension: the user should have to enable it. That avoids surprises when migrating to std::ratio (as it is defined presently). _____ Rob Stewart robert.stewart@sig.com 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.

----- Original Message ----- From: "Stewart, Robert" <Robert.Stewart@sig.com> To: <boost@lists.boost.org> Sent: Thursday, October 07, 2010 2:40 PM Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Ratio Library

vicente.botet wrote:

...

From: "Stewart, Robert" <Robert.Stewart@sig.com>

...

...

...

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

I figured as much, but the names are clear without the redundancy. To what does "numerator" apply if not to the ratio? You should shorten the names when clarity isn't sacrificed. That means that BOOST_RATIO_RATIO_ADD might be acceptable, though I don't find such an example in ratio.hpp. Indeed, the three names I find would be quite clear without the redundancy.

I will try remove the reduncdant RATIO_RATIO.

...

...

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.

Here's a list of lines which refer to intmax_t without the "boost::" scope:

Note that all of those lines appear within the boost namespace, so while they may well refer to boost::intmax_t, they could cause an ambiguity with std::intmax_t if a using declaration or directive pulled it into the global namespace.

Oh, I see the danger now. I will prefix all the uses of intmax_t.

...

...

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.

I was referring to what would appear when the user violates the assertions. I wasn't concerned about the names you chose for your implementation.

I'll make the modification.

...

...

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?

I did in what you quoted next. Your names appear in boost::detail, so if another library did the same, including both headers could cause conflicts.

I see. In order to avoid issues on the detail namespace I will move all to the ratio_detail namespace.

...

...

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.

Perhaps the math version should be expanded?

I will see what can be done with John.

...

I could move these to a file in the detail directory if you prefer.

I was just thinking of folks who wander into ratio.hpp. The code would be clearer with less of the implementation details at first. Making that change is purely aesthetic, however, and you should do it only if you think the result worthwhile. What I asserted was merely opinion and I didn't mean for you to take it otherwise.

Waiting for modification in Boot.Math, I will extract them from the ratio file and put in a specific implementation file.

...

...

Consider moving the empty function bodies to a separate line.

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

Search for "{}" and move them to their own line.

Oh I see. I can do it.

...

...

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?

As I understand it, one can use std::ratio_add<R1,R2> in a context requiring a std::ratio<T1,T2>. To do something similar with Boost.Ratio means using boost::ratio<R1,R2>::type.

Sorry. I should understood your sentence before. Yes, this is a flaw of the current design respect to the C++0x. I see this difference relly recently, and I noted it in the limitation. I though about the using inheritance to emulate the template typedefs, but it was too late to get it before the review. I will try to implement them this way so the interface don't change respect to C++0x.

...

...

s/double count/strain one's eyes carefully counting/ OK if this is clearer.

I didn't understand "double count" at first, and the alternative is more evocative.

I will take it.

...

...

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?

Are you asking why not keep the comments or saying my suggestion is an acceptable alternative that you're willing to adopt?

The later. Thnaks, Vicente