boostorg / rational

Boost.org rational module
http://boost.org/libs/rational
Boost Software License 1.0
20 stars 40 forks source link

Creating an arbitrary precision rational with negative denominator throws exception #27

Open Schniwarixl opened 5 years ago

Schniwarixl commented 5 years ago

When using a rational with an arbitrary precision integer as given by...

typedef boost::multiprecision::number<boost::multiprecision::cpp_int_backend<0, 0, boost::multiprecision::signed_magnitude, boost::multiprecision::checked>> RationalIntType;

typedef boost::rational Rational;

The following will throw an exception...

Rational test(-2, -4);

This is due to the line

if (den < -(std::numeric_limits::max)()) { BOOST_THROW_EXCEPTION(bad_rational("bad rational: non-zero singular denominator")); }

in "template void rational::normalize()"

where std::numeric_limits::max() gives 0. Therefore all negative denominators will throw an exception.

In previous boost version there was no test against max().

psavouli commented 4 years ago

This bug was introduced with the fix for https://github.com/boostorg/rational/issues/18 released in boost 1.68.

It can be reproduced with the following single line (throws at construction):

boost::multiprecision::cpp_rational rational(1, -2);

As Schniwarixl commented, the problem is the the check using std::numeric_limits<T>::max before ensuring that the denominator is positive. I believe this check is there to handle edge cases like -(-2147483648) which, for example, is not valid for a 32bit signed integer (maximum positive value is 2147483647).

However, std::numeric_limits<T>::max is only meaningful for bounded types. In the case of boost arbitrary precisions types, std::numeric_limits<T>::max will return the default constructed value. For a cpp_int specifically, this is 0.

A solution is to check if the type is bounded or not, using std::numeric_limits<T>::is_bounded before calling std::numeric_limits<T>::max.

psavouli commented 4 years ago

Opened pull request with the proposed fix plus a unit test: https://github.com/boostorg/rational/pull/41.

mekhontsev commented 3 years ago

The code below throws exception:

boost::rational<boost::multiprecision::cpp_int> r(1, -1)

Solution: the following line in the boost::rational::normalize() :

if (den < -(std::numeric_limits<IntType>::max)()) {

must be replaced with:

if (den == -den) {
PolarNick239 commented 3 years ago

+1, updated from boost 1.64 to 1.77 and encountered the same error.

Workarounded it with

    if (d < 0) {
        n *= -1;
        d *= -1;
    }
        boost::rational<boost::multiprecision::cpp_int> rational(n, d);
Bolpat commented 1 year ago

I found a comment in my company’s code base referencing this issue by link. This seems to be solved (at least in 1.80.0). Why is the issue still open?