For loop should be well-formed-CodePudding

MISRA C-2012 Control Flow Expressions (MISRA C-2012 Rule 14.2)

misra_c_2012_rule_14_2_violation: The expression i used in the for loop clauses is modified in the loop body.

  for( i = 0; i < FLASH; i   )
  {
    if( name.see[i] == 0xFF )
    {
      name.see[ i ] = faultId | mnemonicType;

modify_expr: Modifying the expression i.

      i =  FLASH-1; /* terminate loop */
    }    
  }

CodePudding user response：

You aren't allowed to modify the loop iterator i inside the loop body, doing so is senseless and very bad practice. Replace the obfuscated code i = FLASH-1; with break;.

CodePudding user response：

This for loop

  for( i = 0; i < FLASH; i   )
  {
    if( name.see[i] == 0xFF )
    {
      name.see[ i ] = faultId | mnemonicType;
      i =  FLASH-1; /* terminate loop */
    }    
  }

is not clear for readers of the code.

Even if you will write

  for( i = 0; i < FLASH; i   )
  {
    if( name.see[i] == 0xFF )
    {
      name.see[ i ] = faultId | mnemonicType;
      break;
    }    
  }

then using the break statement is not a good approach. Each code block should have one entry point and one exit point.

In fact what you need is to find an element that satisfies the condition

name.see[i] == 0xFF

and is such an element exists then change it.

So it is better to write a while loop instead of the for loop the following way

  i = 0;

  wjile ( i < FLASH && name.see[i] != 0xFF ) i  

  if ( i != FLASH ) name.see[ i ] = faultId | mnemonicType;

The advantage of this approach is that the while loop as is can be formed as a body of a function that finds an element in the array. It will be enough just to add the return statement

return i;

CodePudding user response：

Misra C 2004 rule 13.6 (14.2 in the 2012 edition) says

Numeric variables being used within a for loop for iteration counting shall not be modified in the body of the loop.

The code modifies i in order to break for the loop (as the comment confirms). This is a violation of the rule

Misra C 2004 rule 14.6 says:

For any iteration statement there shall be at most one break statement used for loop termination.

Hence you can replace the offending code with a simple break statement:

    for (i = 0; i < FLASH; i  ) {
        if (name.see[i] == 0xFF) {
            name.see[i] = faultId | mnemonicType;
            break;
        }
    }

Yet Misra says you can only do this if there is a single break statement in the loop. What if you want to test 2 different cases, handle them differently and break the loop on each of them. Using 2 break statements seems an obvious choice, but for compliance you would need to add an extra variable do_break, set it in the places where you want to break and test it just once at the end of the body to execute the break statement. Not a very good practice IMHO...

Note these facts about Misra C coding standards:

Misra renumbered the rules from one edition to the next, a necessary change creating some confusion.
The rules are not available in open source. This would help spread some good practices, but arguably prevented some questionable ones.