Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix uint_parser<T(signed)> overflow problem #297

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

wanghan02
Copy link
Contributor

@wanghan02 wanghan02 commented Nov 27, 2017

According to spirit document,

"All numeric parsers check for overflow conditions based on the type T the corresponding uint_parser<> has been instantiated with. If the parsed number overflows this type the parsing fails. Please be aware that the overflow check is not based on the type of the supplied attribute but solely depends on the template parameter T."

the valid input range of x3::uint_parser<T> should be 0 - std::numeric_limits<T>::max(). However, the current implementation takes values from std::numeric_limits<T>::max()+1 - std::numeric_limits<std::make_unsigned_t<T>>::max() also as valid input when T is a signed integral.

Example:
x3::uint_parser<signed char> should parse 0-127, but with current implementation 128-255 also works.

Test cases are added.

wanghan02 and others added 2 commits November 27, 2017 17:47
According to spirit document,

  "All numeric parsers check for overflow conditions based on the type T the corresponding uint_parser<> has been instantiated with. If the parsed number overflows this type the parsing fails. Please be aware that the overflow check is not based on the type of the supplied attribute but solely depends on the template parameter T."

the valid input range of x3::uint_parser<T> should be 0 - std::numeric_limits<T>::max(). However, the current implementation takes values from std::numeric_limits<T>::max()+1 - std::numeric_limits<std::make_unsigned_t<T>>::max() also as valid input.
@djowel
Copy link
Member

djowel commented Dec 1, 2017

Is this for X3 or Qi? Or it seems both?

@Kojoley Kojoley changed the title fix x3::uint_parser<T(signed)> overflow problem fix uint_parser<T(signed)> overflow problem Dec 1, 2017
@Kojoley
Copy link
Collaborator

Kojoley commented Dec 1, 2017

Now for both.

@djowel
Copy link
Member

djowel commented Dec 1, 2017

Great!

@djowel
Copy link
Member

djowel commented Dec 1, 2017

looks good to me.

@wanghan02
Copy link
Contributor Author

@Kojoley Thanks!

@Kojoley Kojoley merged commit 9fc87a8 into boostorg:develop Dec 1, 2017
@Kojoley
Copy link
Collaborator

Kojoley commented Dec 1, 2017

Thanks for the patch @wanghan02!

@wanghan02 wanghan02 deleted the thinkcell_uint_parser_overflow branch December 4, 2017 09:08
@mwpowellhtx
Copy link

@djowel I'm not positive I've just bumped into this, or a similar, kerfuffle as well in Qi. Base 10 parses negative values just fine, but has difficulty dealing with negative base 16 (hex) and base 8 (octal) integers. See example code for illustration.

@Kojoley
Copy link
Collaborator

Kojoley commented Nov 12, 2018

I'm not positive I've just bumped into this, or a similar

The problem in your example has nothing with the PR (it was about uint_parser while you are using int_parser), you can confirm that by testing on previous versions. What you are trying to achieve was never supported (int parsers explicitly expects minus sign in front of a number to threat it as a negative

bool hit = extract_sign(first, last);
if (hit)
hit = extract_neg_type::parse(first, last, attr_);
else
hit = extract_pos_type::parse(first, last, attr_);
). It is also a platform dependent thing, but should not be that hard for you to implement yourself.

Here is how I would done it (for two's complement; if I was needed to):

template <typename T>
bool parse_int_hex(..., T& out)
{
    make_unsigned_t<T> value;
    if (!parse(..., value)) return false;
    constexpr auto max_positive = static_cast<make_unsigned_t<T>>(std::numeric_limits<T>::max());
    if (value > max_positive)
        out = static_cast<T>(value - max_positive) + std::numeric_limits<T>::min() - 1;
    else
        out = static_cast<T>(value);
    return true;
}

https://wandbox.org/permlink/58jpePpKXgOPvCc3

@mwpowellhtx
Copy link

@Kojoley Good to know, thank you for clarifying that. Apologies for the detour.

@mwpowellhtx
Copy link

@Kojoley So, just to clarify, while I can boost::format negative values to hex, for instance, just fine, the parsing side of the equation does not support that, I would need to produce a kind of signed-int attribute involving any signage that were part of the input stream. i.e. in C++ pseudo code:

enum num_sign_t : char { sign_minus = '-', sign_plus = '+' };
struct int_t {
    boost::optional<num_sign_t> sign;
    std::intmax_t _value;
    std::intmax_t get_value() const;
}

It's a bit of work, but it is more consistent with the language specification I am dealing with at the moment. Would also need to be cautious of corner cases like "negative max-int", whose edge case could really only support no more than "negative (max-int + 1)", I think. Along these lines. Could be a corner case unless I can specify the maximum value of the "positive" side.

Okay, well, enough of this tangent... Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants