Developer forums (C::B DEVELOPMENT STRICTLY!) > Development

GCC 4.3 Warnings

<< < (2/3) > >>

killerbot:
well, if you look at the code above in tinyxmlparser.cpp


--- Code: --- while ( *p )
{
                        // blablablablablablablabla

if ( IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' ) // Still using old rules for white space.
++p;
else
break;
}

--- End code ---

==> so first *p and then if(.....) ===> *p && (....)


you will see that this is not what was intended :

--- Code: --- while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
  ++p;

--- End code ---

and it should be :

--- Code: --- while ( *p && (IsWhiteSpace( *p ) || *p == '\n' || *p =='\r') )
++p;

--- End code ---

only continue the rest of the test if *p is not 0;

This bug will hardly occur or have severe damage (if any0, but it is for sure wrong. And therefor a very good thing that GCC warns about this.

EDIT : I just looked at the patch part from "rhf", and that solution is not correct in my opinion.

rhf:

--- Quote from: thomas on January 19, 2009, 08:50:44 am ---Nevertheless, silly as the warnings are, and redundant as the extra parentheses are in most cases, they may make code easier to understand for some people in some cases.

--- End quote ---

--- Quote from: killerbot on January 19, 2009, 01:17:27 pm ---This bug will hardly occur or have severe damage (if any0, but it is for sure wrong. And therefor a very good thing that GCC warns about this.
EDIT : I just looked at the patch part from "rhf", and that solution is not correct in my opinion.

--- End quote ---
Based on your post, I certainly agree. All I tried to do in the patch was make explicit the precedents in the existing code. Some of the changes are a little bit tricky. I was especially concerned with the two that I did not patch. The precedence rules are clear enough, but the indentations make me wonder if the programmer intended something else.

grischka:

--- Code: ---while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
    ++p;

--- End code ---

Actually it doesn't make any difference whether or where you put additional parentheses in there, because *p == '\n' of course includes *p != '\0' and also IsWhiteSpace() (which relies on isspace) includes *p != '\0' as well, moreover explicitely includes '\r\n' too, which means the check for '\r\n' is twice redundant because isspace includes '\r\n' already tpp.

As bottom line, the above code is logically identical to just

--- Code: ---while (IsWhiteSpace(*p))
    ++p;

--- End code ---

killerbot:
absolutely correct, simplification is better ;-)

rhf:

--- Quote from: grischka on January 19, 2009, 07:37:41 pm ---As bottom line, the above code is logically identical to just

--- Code: ---while (IsWhiteSpace(*p))
    ++p;

--- End code ---

--- End quote ---
Neat! Incorporated in patch.

Navigation

[0] Message Index

[#] Next page

[*] Previous page