Wrong overflow checking with uint_parser<int>

[Giulio Franco <giulio_franco@…>]

Running the following code will produce "Parsed -2147483648" on the standard output.

#include <iostream>
#include <boost/spirit/include/qi.hpp>
#include <boost/spirit/include/qi_parse_attr.hpp>
using namespace boost::spirit::qi;
int main() {
    char const test[] = "2147483648";   //INT_MAX + 1
    char const* begin = test;
    int n;
    if ( parse( begin, test + sizeof(test) - 1, uint_parser<int>(), n ) ) {
        std::cout << "Parsed " << n << std::endl;
    } else {
        std::cout << "Parse failed" << std::endl;
    }
    return 0;
}

Since I'm requesting a uint_parser, I assume I should never get a negative value. This is what happens, for instance, with uint_parser<double>, which will reject negative values.

The issue probably lies in spirit/home/qi/numeric/numeric_utils.hpp, where extract_uint::call invokes

extract_type::parse(first, last, detail::cast_unsigned<T>::call(attr_))

but later positive_accumulator::add, will check the overflow condition for the type of its argument (which will be unsigned int, due to the previous cast), rather than for the template type T of the extract_uint struct, which is correctly int.

The call stack I'm talking about is: parse -> any_uint_parser::parse -> extract_uint::call -> extract_int::parse -> extract_int::parse_main -> int_extractor::call -> int_extractor::call (with mpl::true_) -> positive_accumulator::add

[Joel de Guzman]

You are requesting for a unit parser but you give it an int? uint_parser, for performance reasons, does not test for wrap around -- which is what happens here. it is assumed that your type T does not wrap around as int does. perhaps we should be strict and test it with a static assert instead, but there's no generic way to know if T does or does not wrap around. The alternative, that you seem to be suggesting is to check for wrap, but that would penalise the majority of uses.

No this is not a bug. And I don't think it should be "fixed". The best we can do is document this precondition that T should not wrap around.

[Giulio Franco <giulio_franco@…>]

According to the docs, uint_parser is supposed to parse an unsigned value, it doesn't make any restriction on the type it will read the value into. In fact, you can also create a uint_parser<double>, and in that case int_extractor::call(Char, size_t, T&) will use a different implementation, that just adds the digits without checking for overflow.

As I wrote, the current implementations already checks for wrap around, by using std::numeric_limits<T>::max in

template<typename T, typename Char>
positive_accumulator::add(T&, Char, mpl::true_)

The only issue is that it's using the wrong T. It uses numeric_limits<unsigned int>, because it is passed an unsigned int&. If it used numeric_limits<signed int> with the very same algorithm it would probably work perfectly. It looks like it only involves template work, which means no performance impact for those who aren't using it.

[Joel de Guzman]

OK, if you have a patch, then I'll consider it. Please post a pull request to ​https://github.com/boostorg/spirit/tree/develop, making sure that all the tests pass.

(Yes, the docs does not give any restrictions on T at the moment, but if there's no optimal way to make it work with wrapping ints, then I'm inclined to make it an unchecked precondition. T == double is irrelevant because double does not wrap. You can also use saturating ints that do not wrap, for example).

[Nikita Kniazev <nok.raven@…>]

Fixed in ​https://github.com/boostorg/spirit/pull/297