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 signed/unsigned mismatch warning on gcc. #140

Merged
merged 2 commits into from
Jul 27, 2015

Conversation

Lastique
Copy link
Member

No description provided.

@@ -316,7 +316,7 @@ namespace boost { namespace spirit { namespace qi { namespace detail
if (!Accumulate)
{
// skip leading zeros
while (it != last && *it == '0' && leading_zeros < MaxDigits)
while (it != last && *it == '0' && (MaxDigits < 0 || leading_zeros < static_cast< std::size_t >(MaxDigits)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add the logic with MaxDigits < 0 ? I don' think this will ever happen in this code branch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Jul 14, 2015 at 12:50 PM, Joel de Guzman notifications@github.com
wrote:

In include/boost/spirit/home/qi/numeric/detail/numeric_utils.hpp
#140 (comment):

@@ -316,7 +316,7 @@ namespace boost { namespace spirit { namespace qi { namespace detail
if (!Accumulate)
{
// skip leading zeros

  •            while (it != last && *it == '0' && leading_zeros < MaxDigits)
    
  •            while (it != last && *it == '0' && (MaxDigits < 0 || leading_zeros < static_cast< std::size_t >(MaxDigits)))
    

Why did you add the logic with MaxDigits < 0 ? I don' think this will ever
happen in this code branch.

  1. I didn't find any restrictions on the MaxDigits values. From the
    specialization below it seems MaxDigits == -1 is allowed and means "no
    limit". I don't know what behavior should be for other negative values, so
    I extended the same semantics on them as well. Let me know if this
    assumption is wrong.
  2. I found only one specialization of extract_int, which is the one below -
    for MinDigits == 1 and MaxDigits == -1. This specialization won't be used
    for e.g. MinDigits == 3 && MaxDigits == -1 which means the generic template
    will be used and the 'while' condition should be prepared for negative
    values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. You are right. I'll review the code some more. It's been a while.

@Lastique
Copy link
Member Author

Ping? Would be great if this fix made it to 1.59.

@djowel
Copy link
Member

djowel commented Jul 27, 2015

I'm not comfortable with the code change and possible behaviour change, unless there are some tests that exploit the new code, if it is indeed a bug without it. In that case, then perhaps it's best to have that in another PR.

@Lastique
Copy link
Member Author

The original code was comparing size_t with an int. The negative int would be converted to a large unsigned number in this case, so the behavior was "to skip up to 4 billion something leading zeros". The current behavior is "to skip all leading zeros", which is the same for all practical purposes.

I can change this PR to explicitly cast the int to size_t, which will remove the warning and keep the 4 billion quirk. It just doesn't make much sense to me. Should I do it?

@djowel
Copy link
Member

djowel commented Jul 27, 2015

OK, I studied the code a bit more. Pardon my being slow. MaxDigits < 0 is indeed a good idea and that branch will be optimized away by the compiler quite nicely (being const).

djowel added a commit that referenced this pull request Jul 27, 2015
Fix signed/unsigned mismatch warning on gcc.
@djowel djowel merged commit 5bc397c into boostorg:develop Jul 27, 2015
@djowel
Copy link
Member

djowel commented Jul 27, 2015

Thanks, Andrey.

@Lastique Lastique deleted the patch-1 branch July 27, 2015 13:19
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

2 participants