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-07 08:40:37

vicente.botet wrote:
> From: "Stewart, Robert" <Robert.Stewart_at_[hidden]>
>
> > 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_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.