GCC 4.3 Warnings

[rhf]

Hi guys,

I've been using CB for sometime now and feel a little guilty because I have not been contributing to your development work. Recently I did a fresh rebuild of Codeblocks with TDM's gcc4.3.2 and noticed quite a few warnings, such as "suggest parentheses around && within ||" and "suggest explicit braces to avoid ambiguous 'else'".

To help out, I thought I would prepare a patch to eliminate these warnings. However, most of these are in tinyxml, squirrel, or wxscintilla. Does it make sense to make changes in these files, or should such changes be directed to the developers of the programs?

Also, if there is a "real" potential ambiguity in these files, should this be brought to the attention of the CB team or to the original developer?

[killerbot]

we are already fixing them, no at a fast pace. So any conribution might help ;-)
I know there's one in tinyxml, which is really a bug in tinyxml (which might be hard to occur though)

[rhf]

OK. I submitted patch #2652 that fixes all but two of the warnings - maybe this will be helpful.
In a comment to the patch I mention the two warnings that I left alone because I was unsure of the intent of the code.

[Jenna]

The first hunk of the patch (in tinyxmlparser.cpp) is most likely wrong, even if it manifests the way tinyxmlparser works for years.

I searched in the cvs-sources and found the revision where the (most likely) wrong behaviour came in.

Here are the original functions, before and after the change:

Code

http://tinyxml.cvs.sourceforge.net/viewvc/tinyxml/tinyxml/tinyxmlparser.cpp?revision=1.46&view=markup


  260 const char* TiXmlBase::SkipWhiteSpace( const char* p )
  261 {
  262 	if ( !p || !*p )
  263 	{
  264 		return 0;
  265 	}
  266 	while ( p && *p )
  267 	{
  268 		// Skip the stupid Microsoft UTF-8 Byte order marks
  269 		if (	*(p+0)==(char) 0xef
  270 			 && *(p+1)==(char) 0xbb
  271 			 && *(p+2)==(char) 0xbf )
  272 		{
  273 			p += 3;
  274 			continue;
  275 		}
  276 		else if(*(p+0)==(char) 0xef
  277 			 && *(p+1)==(char) 0xbf
  278 			 && *(p+2)==(char) 0xbe )
  279 		{
  280 			p += 3;
  281 			continue;
  282 		}
  283 		else if(*(p+0)==(char) 0xef
  284 			 && *(p+1)==(char) 0xbf
  285 			 && *(p+2)==(char) 0xbf )
  286 		{
  287 			p += 3;
  288 			continue;
  289 		}
  290 
  291 		if ( IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )		// Still using old rules for white space.
  292 			++p;
  293 		else
  294 			break;
  295 	}
  296 
  297 	return p;
  298 }


http://tinyxml.cvs.sourceforge.net/viewvc/tinyxml/tinyxml/tinyxmlparser.cpp?revision=1.47&view=markup

 289 const char* TiXmlBase::SkipWhiteSpace( const char* p, TiXmlEncoding encoding )
  290 {
  291 	if ( !p || !*p )
  292 	{
  293 		return 0;
  294 	}
  295 	if ( encoding == TIXML_ENCODING_UTF8 )
  296 	{
  297 		while ( *p )
  298 		{
  299 			// Skip the stupid Microsoft UTF-8 Byte order marks
  300 			if (	*(p+0)==(char) 0xef
  301 				 && *(p+1)==(char) 0xbb
  302 				 && *(p+2)==(char) 0xbf )
  303 			{
  304 				p += 3;
  305 				continue;
  306 			}
  307 			else if(*(p+0)==(char) 0xef
  308 				 && *(p+1)==(char) 0xbf
  309 				 && *(p+2)==(char) 0xbe )
  310 			{
  311 				p += 3;
  312 				continue;
  313 			}
  314 			else if(*(p+0)==(char) 0xef
  315 				 && *(p+1)==(char) 0xbf
  316 				 && *(p+2)==(char) 0xbf )
  317 			{
  318 				p += 3;
  319 				continue;
  320 			}
  321 
  322 			if ( IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )		// Still using old rules for white space.
  323 				++p;
  324 			else
  325 				break;
  326 		}
  327 	}
  328 	else
  329 	{
  330 		while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
  331 			++p;
  332 	}
  333 
  334 	return p;
  335 }

I left them in fulllength, to make sure I did not oversee anything.
Please have a look.

[thomas]

Also, if there is a "real" potential ambiguity in these files, should this be brought to the attention of the CB team or to the original developer?

No worries. Those warnings are somewhat silly, as there's no real ambiguity. The language standard is very clear about where an else belongs to, and it's also very clear about what something like while(foo = *bar) or if(a && b & c) means.
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.

[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;
		}

==> 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;

and it should be :

Code

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

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]

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.

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.

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;

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;

[killerbot]

absolutely correct, simplification is better ;-)

[rhf]

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

Code

while (IsWhiteSpace(*p)) 
    ++p;

Neat! Incorporated in patch.

[thomas]

The precedence rules are clear enough, but the indentations make me wonder if the programmer intended something else.

We will never know, but seeing that this is part of tinyxml which works just fine, it at least appears that it is probably what was intended.
I didn't mean to criticise your patch or imply that you don't know the precedence rules. What I'm saying is that I perceive those "suggest parenthese" warnings as quite silly, as I'm getting one or two of them in my own code every day, and unlike most other warnings (which are useful) it has never pointed to an actual ambiguity or typo, it has only ever added noise. Such as in having to turn while(foo = *bar) into while((foo = *bar)) only to make the compiler happy.

[thomas]

Forgot to say: You should forward this patch (the last version) to the tinyxml team so it can be reviewed and possibly applied upstream.

Tampering with third party code can have undesirable effects, as history has proven. It's never a mistake to communicate with the guy who wrote the package initially. Not only does that make the possibility of a regression due to misinterpretation less likely, but also it will let the entire user base profit from the patch if it is accepted.

[killerbot]

don't worry, I will communicate to Lee, I send him some suggestions/changes from time to time.