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.