Code::Blocks Forums

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?

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)

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.

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:
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.

Quote from: rhf on January 18, 2009, 10:10:31 PM
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.

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


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 :

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


and it should be :

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.

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.

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.

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.


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

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


absolutely correct, simplification is better ;-)

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

Code Select Expand


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


Neat! Incorporated in patch.

Quote from: rhf on January 19, 2009, 06:52:05 PMThe 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.

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 (http://www.links.org/?p=327) 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.

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