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