For loop should be well-formed

Question

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 */
    }    
  }

MISRA is telling you there are *two* loop modifiers; one in the `for` statement, one in the marked expression. Please see [MISRA 2012 Rule 14.2](https://stackoverflow.com/questions/50691602/misra-2012-rule-14-2) *The rationale for Rule 14.2 shows that this Rule is intended to restrict for loops, stopping "clever" uses, and thus make code easier to review and analyse*. Since MISRA is about writing safe code, that is **bad practice**. Use `break;` instead. — Weather Vane, Dec 02 '21 at 08:22
Dare I suggest that dubious practice such as this is *exactly* the reason for the Rule? The language gives you the answer: `break` which is OK to use in a controlled way. — Andrew, Dec 03 '21 at 08:43

score 3 · Answer 1 · answered Dec 02 '21 at 08:29

3

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

answered Dec 02 '21 at 08:29

Lundin

195,001
40
254
396

break; doesn't seem to work – Sharon Deborah Dec 03 '21 at 05:14
1

@SharonDeborah It works just fine. – Lundin Dec 03 '21 at 07:06

chqrlie · Answer 2 · 2021-12-09T11:09:00.973

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 finish the for 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 and still conform:

    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.

MISRA-C:2012 (the current active version) renumbered the rules because it's a major revision with lots of changes. Almost every rule has been changed. This particular one 14.2 regarding `for` loops has been expanded a lot, to disallow side-effects at certain places and in clause 2 not to use _any_ object modified in the `for` loop body other than the iterator and control flags etc. Basically enforcing "Keep It Simple Stupid" principle for `for` loops. — Lundin, Dec 02 '21 at 10:59
As for the single `break` rule it has been relaxed to advisory, but it's a rather sound rule IMO. — Lundin, Dec 02 '21 at 11:01
@SharonDeborah: you did not post the rest of the code, is `i` used after the `for` loop? The only difference between the posted code and that of the answer is the final value of `i`. — chqrlie, Dec 03 '21 at 07:47

Vlad from Moscow · Accepted Answer · 2021-12-02T08:37:35.367

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;

For loop should be well-formed

3 Answers3