Modified rule 14.1 by swasti16 · Pull Request #1267 · cppcheck-opensource/cppcheck

swasti16 · 2018-05-28T13:12:07Z

I have tried implementing rule 14.1 for while loop also.

danmar

I think it could be more robust.

danmar · 2018-05-28T15:59:10Z

            lpar.astOperand2.astOperand2.astOperand2]

+def getWhileLoopExpressions(whileToken):
+    if not whileToken or whileToken.str != 'while':


I suggest :

if not simpleMatch(whileToken, 'while ('): return None

danmar · 2018-05-28T16:00:14Z

+        return None
+    counter = lpar.astOperand2.astOperand1.str
+    token = lpar
+    while (token and token.str != "{"):


use the token.link:

rpar = lpar.link

and then this check might be appropriate

if not simpleMatch(rpar, ') {'): return None

danmar · 2018-05-28T16:02:50Z

+    lpar = whileToken.next
+    if not lpar or lpar.str != '(':
+        return None
+    if not lpar.astOperand2.astOperand1:


I think you should check what lpar.astOperand2 is. if the code is:

while (a<100 || b > 100)

then the counter will be <

danmar · 2018-05-28T16:05:46Z

+                return lpar.astOperand2
+            elif token.str == counter and token.astParent and token.astParent.str in {'++', '--'}:
+                return lpar.astOperand2
+    elif not token_link and whileToken.previous.str == '}':


The not token_link and is redundant. You can just write:

elif whileToken.previous.str == '}':

swasti16 · 2018-05-29T09:26:39Z

I have made all suggested changes.

danmar

I think it can be more robust still.

danmar · 2018-05-29T10:33:15Z

-        while (token and token != token_link.link):
+    rpar = lpar.link
+    counter_token = lpar.next
+    while (counter_token != rpar):


I would not loop like this. Now if the condition is:

while (abc.a < 10)

... then as I read it, the counter will be "abc"

How do you think this should be handled:

while (x < y)

... Do you think that x is a counter? Do you think that y is a counter? I think that either both should be counters or none should be counters.

I would recursively look at the condition from the parent..

def findCounterTokens(cond): if cond.str in ['&&' , '||']: c = findCounterTokens(cond.astOperand1) c.extend(findCounterTokens(cond.astOperand2)) return c ret = [] if cond.isComparisonOp and cond.astOperand1.isName and cond.astOperand2.isNumber: ret.append(cond.astOperand1) elif cond.isComparisonOp and cond.astOperand2.isName and cond.astOperand1.isNumber: ret.append(cond.astOperand2) return ret

and it will be called like this:

counter_tokens = findCounterTokens(lpar.astOperand2)

danmar · 2018-05-29T10:37:02Z

+                    expr = hasFloatComparison(counter_str.astParent)
+                    if expr:
+                        return True
+    elif simpleMatch(whileToken.previous, '} while'):


I would recommend that you check that it's a do-while statement also.

Either:

elif simpleMatch(whileToken.previous, '} while') and simpleMatch(whileToken.previous.link.previous, 'do {')

Or (untested code, you probably need to adjust it a little):

elif simpleMatch(whileToken.previous, '} while') and whileToken.previous.scope.type == 'DO':

danmar

various comments

danmar · 2018-05-30T10:52:21Z

            lpar.astOperand2.astOperand2.astOperand2]


+def findCounterTokens(cond):


it seems to me that you can cleanup and refactor this findCounterTokens.

danmar · 2018-05-30T10:56:24Z

+
+
+def getFloatComparision(floatCompOperator):
+    while not floatCompOperator.isComparisonOp:


this loop looks unsafe , are you sure that there will always be a parent that is a comparison op? I do not feel sure.

danmar · 2018-05-30T11:00:36Z

+        return None
+    if simpleMatch(rpar, ') {'):
+        token = rpar.next
+        while (token != rpar.next.link):


this loop is more or less copy pasted and written twice. please refactor it so there is only 1 loop that looks at the while body.

I still think this is true.

I envision something like:

whileBodyStart = None if simpleMatch(rpar, ') {'): whileBodyStart = rpar.next elif simpleMatch(whileToken.previous, '} while') and simpleMatch(whileToken.previous.link.previous, 'do {'): whileBodyStart = whileToken.previous.link else: return False ... check the while loop body ...

Wow!!
I never thought this way!
Thanks a lot :)

danmar · 2018-05-30T11:02:40Z

        return hasFloatComparison(expr.astOperand1) or hasFloatComparison(expr.astOperand2)
+    if expr.isArithmeticalOp:
+        if expr.astOperand1.isFloat or expr.astOperand2.isFloat:
+            return expr.astOperand1.isFloat or expr.astOperand2.isFloat


this expression is copy pasted so it can be changed to return True

danmar · 2018-05-30T11:06:33Z


 void misra_14_1() {
  for (float f=0.1f; f<1.0f; f += 0.1f){} // 14.1
+  f = 0.0f;


this f seems to be out of scope. maybe f should be a local variable in this function instead so it can be used in the while loop conditions.

danmar · 2018-05-30T11:32:54Z

+    return ret
+
+
+def getFloatComparision(floatCompOperator):


Since this only returns True or False I think that some is... or has... name is better than get....

danmar · 2018-05-30T11:33:51Z

+            ret.extend(findCounterTokens(cond.astOperand2))
+
+    if cond.isComparisonOp:
+        if cond.astOperand1.isName and cond.astOperand2.isNumber:


this logic seems to be copy pasted from isArithmeticalOp, it can be written only once.

danmar · 2018-05-30T11:36:13Z

+    if cond.isArithmeticalOp:
+        if cond.astOperand1.isName and cond.astOperand2.isNumber:
+            ret.append(cond.astOperand1)
+        elif cond.astOperand2.isName and cond.astOperand1.isNumber:


Do you need all these conditions that looks if cond.astOperand1 is a name or not? why not just:

if cond.astOperand1.isName: ret.append(cond.astOperand1) if cond.astOperand2.isName: ret.append(cond.astOperand2) ....

danmar · 2018-05-30T11:41:04Z

        return False
    if expr.isLogicalOp:
        return hasFloatComparison(expr.astOperand1) or hasFloatComparison(expr.astOperand2)
+    if expr.isArithmeticalOp:


it seems to me that with this code:

a = b + 5.0;

you will say that there is a float comparison. I do not agree we should return True for this code.

This function used to check if there was a comparison.. and if that was seen it was checked if one of the operands was a float type.

I included this so that while((10.0f*c) > (a*b)) can return True.

As far as I see the old code should return True for that. Maybe I need to look into this. Did the cppcheckdata.astIsFloat() return False for the expression 10.0f * c? Does it matter if c is declared?

danmar · 2018-05-30T11:47:42Z

 void misra_14_1() {
  for (float f=0.1f; f<1.0f; f += 0.1f){} // 14.1
+  f = 0.0f;
+  while ((a<100.0f) || (b > 100)) //14.1


should it warn here if a is an int variable? I am just asking, I don't know.

I think that according to this example in the MISRA pdf, there should not be a warning for int loop counters:

float32_t f; uint32_t u32a; f = read_float32 ( ); do { u32a = read_u32 ( ); } while ( ( ( float32_t ) u32a - f ) > 10.0f );

Here 'a' is a float variable that's why a warning is raised.
If 'a' is an int variable there should not be any warning.

I do not see the declaration for a.

danmar

minor comments

danmar · 2018-05-31T13:00:25Z

+
+def isFloatCounterInWhileLoop(whileToken):
+    if not simpleMatch(whileToken, 'while ('):
+        return None


I feel that isFloatCounterInWhileLoop should return False/True instead of None/True.

danmar · 2018-05-31T13:02:48Z

+    if not simpleMatch(whileToken, 'while ('):
+        return None
+    lpar = whileToken.next
+    if not lpar.astOperand2.astOperand1:


hmm.. this if not lpar.astOperand2.astOperand1 condition looks redundant to me. Is it to avoid warnings for while (f)?

danmar · 2018-05-31T13:05:06Z

-        exprs = getForLoopExpressions(token)
-        if exprs and hasFloatComparison(exprs[1]):
-            reportError(token, 14, 1)
+        if token.str != 'for' and token.str != 'while':


You do not need to have this code:

if token.str != 'for' and token.str != 'while': continue

You can safely remove these 2 lines of code.

danmar · 2018-05-31T13:05:28Z

+            for counter in findCounterTokens(exprs[1]):
+                if counter.valueType and counter.valueType.isFloat():
+                    reportError(token, 14, 1)
+        if token.str == 'while':


Nit: use elif

danmar · 2018-05-31T13:08:05Z

+            continue
+        if token.str == 'for':
+            exprs = getForLoopExpressions(token)
+            for counter in findCounterTokens(exprs[1]):


It would be a good idea to check that exprs is not None, wouldn't it? also is it best to check that len(exprs) is 3 or is that implied when exprs is not None?

It would be a good idea to check that exprs is not None, wouldn't it?

Yes You are right.

also is it best to check that len(exprs) is 3 or is that implied when exprs is not None?

Yes .. according to me.. if exprs is not None.. len(exprs) is 3

danmar · 2018-05-31T13:10:36Z

+    if not lpar.astOperand2.astOperand1:
+        return None
+    rpar = lpar.link
+    counter = findCounterTokens(lpar.astOperand2)


I think you should rename counter to for instance counterTokens. Please make it obvious that it's a list.

danmar · 2018-05-31T13:12:04Z

+        return None
+    rpar = lpar.link
+    counter = findCounterTokens(lpar.astOperand2)
+    if len(counter) == 0:


It seems safe to remove this code:

if len(counter) == 0: return None

danmar · 2018-05-31T13:14:34Z

+        return None
+    if simpleMatch(rpar, ') {'):
+        token = rpar.next
+        while (token != rpar.next.link):


I still think this is true.

I envision something like:

whileBodyStart = None if simpleMatch(rpar, ') {'): whileBodyStart = rpar.next elif simpleMatch(whileToken.previous, '} while') and simpleMatch(whileToken.previous.link.previous, 'do {'): whileBodyStart = whileToken.previous.link else: return False ... check the while loop body ...

danmar

I like this patch now but I have a few minor nits.

danmar · 2018-06-02T05:35:02Z

+    token = whileBodyStart
+    while (token != whileBodyStart.link):
+        token = token.next
+        for counter_str in counterTokens:


the name counter_str is confusing to me. Since it's not some string. how about counter or counterToken.

danmar · 2018-06-02T05:37:32Z

+        token = token.next
+        for counter_str in counterTokens:
+            if token.isAssignmentOp and token.astOperand1.str == counter_str.str:
+                if counter_str.valueType and counter_str.valueType.isFloat():


hmm.. this condition does not have to be duplicated:

for counter_str in counterTokens: if not counter_str.valueType or not counter_str.valueType.isFloat(): continue ....

danmar · 2018-06-02T05:39:25Z

+                if counter.valueType and counter.valueType.isFloat():
+                    reportError(token, 14, 1)
+        elif token.str == 'while':
+            exprs = isFloatCounterInWhileLoop(token)


the name exprs is confusing to me. I would not use a variable I would just write:

if isFloatCounterInWhileLoop(token):

danmar · 2018-06-04T08:12:45Z

I see nothing more to complain about :-) Thanks!

Modified rule 14.1

f309ca0

danmar requested changes May 28, 2018

View reviewed changes

Made suggested changes

c5e6a40

danmar reviewed May 29, 2018

View reviewed changes

Added findCounterTokens

c87782e

danmar reviewed May 30, 2018

View reviewed changes

Removed hasFloatComparison

b21e5e2

danmar reviewed May 31, 2018

View reviewed changes

Improved isFloatCounterInWhileLoop

dd724c2

danmar reviewed Jun 2, 2018

View reviewed changes

Made all required minor changes

697ca69

danmar merged commit bdb372f into cppcheck-opensource:master Jun 4, 2018

swasti16 deleted the rule_14_1 branch June 14, 2018 04:43

		lpar.astOperand2.astOperand2.astOperand2]


		def findCounterTokens(cond):



		def getFloatComparision(floatCompOperator):
		while not floatCompOperator.isComparisonOp:

Conversation

swasti16 commented May 28, 2018

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swasti16 commented May 29, 2018

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swasti16 May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

swasti16 May 30, 2018 •

edited

Loading