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 12:16:57

----- Original Message ----- 
From: "Stewart, Robert" <Robert.Stewart_at_[hidden]>
To: <boost_at_[hidden]>
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_at_[hidden]>
>>

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