The warning occurs because the logical expression, as it stands, contains an ambiguous mix of AND and OR subexpressions, which the compiler cannot be certain to evaluate with the precedence the programmer intended, without parenthesis to specify this intent.
Your parenthesis just affirm what the compiler will assume, but is that really what the programmer intended? I think not; more likely is:
As it happens, in this case, the ORed subexpressions are redundant, so don't affect the outcome anyway, but the compiler has no way to know that; the entire expression may be safely simplified to:
while( *p && IsWhiteSpace( *p ) )
(ignoring the redundancy within IsWhiteSpace() itself - do you know of any implementation of isspace() which doesn't satisfy the POSIX requirement that '\n' and '\r' are included in the set of whitespace characters?)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Cleaning up the logic is a good thing - I agree the while() is poorly written.
I remember the isspace() problem vaguely. The isspace() function is buggy and incorrectly returns 'false' for n/r on some platform. I forget which one, but that's bug fixes in an mature code base for you. :) TinyXML is run on some strange systems, and I try to accommodate that.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
No issue at all with you accommodating the possibility of a buggy isspace(), which mishandles '\n' or '\r', but you already take care of that in the implementation of IsWhiteSpace(). I've already patched the subset of the source I keep in the mingw-get repository, to remove the redundancy elsewhere: ; applied to:
.
I'll gladly create a patch against your own repository, if you wish.
I get the following warning:
tinyxmlparser.cpp|357|warning: suggest parentheses around '&&' within '||'|
Which is easily fixed by adding parentheses around here:
while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
Changing it from above to this:
while ( (*p && IsWhiteSpace( *p )) || *p == '\n' || *p =='\r' )
Its an easy fix, don't know why the warning should be there when it doesn't have to be.
This issue cropped up on the MinGW Developer's ML, only yesterday:
http://thread.gmane.org/gmane.comp.gnu.mingw.devel/3520
The warning occurs because the logical expression, as it stands, contains an ambiguous mix of AND and OR subexpressions, which the compiler cannot be certain to evaluate with the precedence the programmer intended, without parenthesis to specify this intent.
Your parenthesis just affirm what the compiler will assume, but is that really what the programmer intended? I think not; more likely is:
while( *p && (IsWhiteSpace( *p ) || *p == '\n' || *p == '\r') )
As it happens, in this case, the ORed subexpressions are redundant, so don't affect the outcome anyway, but the compiler has no way to know that; the entire expression may be safely simplified to:
while( *p && IsWhiteSpace( *p ) )
(ignoring the redundancy within IsWhiteSpace() itself - do you know of any implementation of isspace() which doesn't satisfy the POSIX requirement that '\n' and '\r' are included in the set of whitespace characters?)
Cleaning up the logic is a good thing - I agree the while() is poorly written.
I remember the isspace() problem vaguely. The isspace() function is buggy and incorrectly returns 'false' for n/r on some platform. I forget which one, but that's bug fixes in an mature code base for you. :) TinyXML is run on some strange systems, and I try to accommodate that.
Lee,
No issue at all with you accommodating the possibility of a buggy isspace(), which mishandles '\n' or '\r', but you already take care of that in the implementation of IsWhiteSpace(). I've already patched the subset of the source I keep in the mingw-get repository, to remove the redundancy elsewhere: ; applied to:
.
I'll gladly create a patch against your own repository, if you wish.
: http://mingw.cvs.sourceforge.net/viewvc/mingw/mingw-get/ChangeLog?revision=1.6&view=markup "Changelog"
: http://mingw.cvs.sourceforge.net/viewvc/mingw/mingw-get/tinyxml/ "CVS"
Okay - good catch. Once I sat down and looked at the code I saw what you meant. Fixed and checked in.