cppcheck: fix "Suspicious condition and usage" #2755
Conversation
[fontforge/dumppfa.c:2370]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [gdraw/gtabset.c:408]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [fontforge/stemdb.c:4619]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/stemdb.c:4626]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/svg.c:2046]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.
| @@ -2043,7 +2043,7 @@ static void xmlParseColorSource(xmlNodePtr top,char *name,DBounds *bbox, | |||
| *_grad = NULL; *_epat = NULL; | |||
| if ( colour_source==NULL ) | |||
| LogError(_("Could not find Color Source with id %s."), name ); | |||
| else if ( (islinear = xmlStrcmp(colour_source->name,(xmlChar *) "linearGradient")==0) || | |||
| else if ( ((islinear = xmlStrcmp(colour_source->name,(xmlChar *) "linearGradient"))==0) || | |||
There was a problem hiding this comment.
If xmlStrcmp is like strcmp, then the placement of brackets here is now wrong - a zero return value from strcmp indicates a match, so comparing its return value == 0 would indicate the presence of a linear gradient. So you probably want something like
else if ((islinear = (xmlStrcmp(colour_source->name, (xmlChar *) "linearGradient") == 0))There was a problem hiding this comment.
You're right I suppose, I fixed this.
[fontforge/dumppfa.c:2370]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [gdraw/gtabset.c:408]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [fontforge/stemdb.c:4619]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/stemdb.c:4626]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/svg.c:2046]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.
| @@ -4616,14 +4616,14 @@ static void MarkDStemCorner( struct glyphdata *gd,struct pointdata *pd ) { | |||
| for ( i=0; i<pd->prevcnt && !has_stem; i++ ) { | |||
| stem = pd->prevstems[i]; | |||
| if ( !stem->toobig && ( | |||
| ( x_dir && ( hv = IsUnitHV( &stem->unit,true ) == 1 )) || | |||
| ( x_dir && ( (hv = IsUnitHV( &stem->unit,true )) == 1 )) || | |||
| ( !x_dir && hv == 2 ))) | |||
There was a problem hiding this comment.
The bracket fix looks fine, but looking at the statement as a whole, it seems a bit odd that IsUnitHV (and the assignment to hv) is only called after checking if x_dir is true.
I wonder if it should actually be something closer to
if ((hv = IsUnitHV(&stem->unit, true)) == 1 && x_dir) || (!x_dir && hv == 2)I haven't studied the code long enough to know if this is appropriate.
There was a problem hiding this comment.
You're right, in C (on the contrary of C++), we don't know in which order conditions are evaluated, so I think it could be better to put "hv = IsUnitHV( &stem->unit,true )" just after "stem" assignation in both cases.
There was a problem hiding this comment.
BTW, what about merging this patch as it is and making the quoted change on a second patch?
Indeed, this change isn't related to cppcheck.
There was a problem hiding this comment.
You're right, in C (on the contrary of C++), we don't know in which order conditions are evaluated,
Hmm I'm not sure about that - the rearrangement was taking into account short-circuit evaluation (which works in both c/c++). This bug also comes up on coverity (CID1309095)
There was a problem hiding this comment.
Ok, thought it wasn't sure for C.
Thank you for having merged this one!
I'm gonna create a new pull request for the change we've discussed.
* cppcheck: fix "Suspicious condition and usage" [fontforge/dumppfa.c:2370]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [gdraw/gtabset.c:408]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter. [fontforge/stemdb.c:4619]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/stemdb.c:4626]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses. [fontforge/svg.c:2046]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.
[fontforge/dumppfa.c:2370]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter.
[gdraw/gtabset.c:408]: (warning) Suspicious usage of 'sizeof' with a numeric constant as parameter.
[fontforge/stemdb.c:4619]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.
[fontforge/stemdb.c:4626]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.
[fontforge/svg.c:2046]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.