boostorg / spirit

Boost.org spirit module
http://boost.org/libs/spirit
392 stars 161 forks source link

Assertation on big exponents #495

Closed RandomInEqualities closed 5 years ago

RandomInEqualities commented 5 years ago

The following code:

#include <boost/spirit/home/x3.hpp>

#include <string>

namespace x3 = boost::spirit::x3;

int main() {
  const std::string expression = "1e310";
  auto start = expression.cbegin();
  auto end = expression.cend();
  double result;
  x3::phrase_parse(start, end, x3::double_, x3::space, result);
  return 0;
}

Asserts in pow10.hpp:

Assertion failed: dim < sizeof(exponents)/sizeof(double), file boost/spirit/home/x3/support/numeric_utils/pow10.hpp, line 85

Due to the exponentiation table only holding 308 values. In qi the expression is rejected.

One can use their own real policy to workaround it:

template<typename T>
struct RealPolicy : x3::ureal_policies<T> {
    template<typename Iterator>
    static bool parse_exp_n(Iterator& first, Iterator const& last, int& attr_) {
        // boost spirit asserts/reads out of bounds when |exponent| is larger than 308
        const Iterator save = first;
        if (x3::ureal_policies<T>::parse_exp_n(first, last, attr_)) {
            if (attr_ >= -308 && attr_ <= 308) {
                return true;
            } else {
                first = save;
                return false;
            }
        }
        return false;
    }
};
x3::real_parser<double, RealPolicy<double>> const double_ = {};
Kojoley commented 5 years ago

That's a pretty old thing (https://svn.boost.org/trac10/ticket/9400), there are various corner cases in exponents handling...

djowel commented 5 years ago

This has been fixed in Qi in real_impl.hpp. The best strategy is to unify the code in one place. I'll see what I can do. What are the other corner cases, @Kojoley ?

djowel commented 5 years ago

@Kojoley, I'll take this one. Please tell me what other corner cases you are referring to.

djowel commented 5 years ago

@RandomInEqualities Fix is in develop.

RandomInEqualities commented 5 years ago

Thanks, I tested the patch locally, it works well for me.

There is one regression, but I don't really care about that, but it is that "2.1111111e-303" cannot be parsed anymore due to the (exp-frac) in scale(int exp, int frac, T& n), while it looks like scale(int exp, T& n) has some support for exponents lower than min_exp, so it could be parsed before. (But I think just rejecting is totally fine, e.g one would have to do 2.11111...111e-5 with 303 ones to see it in practice).

djowel commented 5 years ago

Good you checked. I'll have a look later. Should be easy. Thanks!

djowel commented 5 years ago

Fixed for big negative exponents. Please try again before I close this.

RandomInEqualities commented 5 years ago

Yes, again thanks a lot. It works great now.