CODEWITHSUNDEEP
cmisra
radoslav006
radoslav006

Reputation: 127

MISRA C 2012 Rule 15.4 and replacing goto's with break's

Regarding to MISRA C 2012 rule 15.4 - "There should be no more than one break or goto statement used to terminate any iteration statement." - is this example correct? Can anyone confirm this with some tool (MISRA checker)?

do {
    retval = do_smth();
    if (retval != OK) {
        break;
    }

    retval = do_smth2();
    if (retval != OK) {
        break;
    }

    retval = do_smth3();
} while (0u);

This is just a concept, but what I am trying here is to replace cascade of goto (unfortunately banned in this case) with cascade of break. My point is that do { } while(0u); is not an iteration statement.
What you think?

Upvotes: 5

Views: 2634

Answers (7)

lonejack
lonejack

Reputation: 27

My poposal:

type do_stuff (void)
{
    type retval;
  
    retval = do_smth();
    if (retval == OK) {
        retval = do_smth2();
    }
    if (retval == OK) {
        retval = do_smth3();
    }
    return retval;
}

It seems more slow on case of (retval = KO) but... Compilers today are very smart. I'm quite sure that in a real compilation the function returns immediatly. Furthermore, with MISRA, don't use goto and only one single return in every function.

Upvotes: 0

Yonatan.Lehman
Yonatan.Lehman

Reputation: 63

In a past project, I introduced a standard macro ER_CHK used throughout the project

Simplified for clarity, this is defined as:

#define ER_CHK(st) if (st != OK) {goto  EXIT;} else ;

You use it as follows:

status_t foo() {
   status_t  st;
   st = bar1(); ER_CHK(st);
   st = bar2(); ER_CHK(st);
   st = bar3(); ER_CHK(st);

   EXIT : ; // ER_CHK calls get here in case of non OK status
   if (st != OK) {
      // put exception handling code here
   }
}

This enforces a single, standard exit - leaving the real code very clean.

I actually define the macro to include a log where the failure was found: e.g.

#define ER_CHK(st) if (st != OK) {log(st,__FILE__,__LINE__); goto  EXIT;} else ;

this provides a log "traceback" as the stack unwinds.

The main point is that this is used throughout the entire project. It allows the normal code to stand out, and there is a standard way to handle exceptions that can be viewed as an idiom, pattern, or language extension.

It makes the normal (non-exception) code much more readable since

  • there are fewer lines
  • only relevant code is visible
  • no need to distinguish between normal "ifs" and exception handling ifs.

For a fuller description see https://forum.misra.org.uk/archive/index.php?thread-368.html and a full article "Exception Handling in Embedded C Programs", C/C++ Users Journal,

Upvotes: 0

huangda1982
huangda1982

Reputation: 17

I don't know if this code is good enough. It works though.

switch(0) {
default:
    retval = do_smth();
    if (retval != OK) {
        break;
    }

    retval = do_smth2();
    if (retval != OK) {
        break;
    }

    retval = do_smth3();
    break;
}

Upvotes: 1

Steve Summit
Steve Summit

Reputation: 47962

I'm no expert on MISRA, so this may be forbidden for other reasons, but in this situation I might be tempted to use something like this:

if((retval = do_smth()) != OK)
    ;
else if((retval = do_smth2()) != OK)
    ;
else if((retval = do_smth3()) != OK)
    ;

No goto, no break, no iteration, no duplicated tests.

The empty statements look weird, it's true, although in practice, there will often be some error-logging code to put there. If naked semicolons aren't to your taste, you could use { }, or { /* do nothing */ }.

Assignments inside conditionals (as in the classic while((c = getchar()) != EOF) loop) can be confusing to the novice (and I suspect MISRA probably bans them for more or less that reason), but in many cases, if you can tolerate them, they can really help to make various other uglinesses melt away.

Upvotes: 0

Lundin
Lundin

Reputation: 214040

First of all, your code does indeed not follow rule 15.4 since you have 3 break inside an iteration statement1). But it is just an advisory one and there's nothing wrong with using multiple breaks like you do as long as the code is readable and easy to follow.

The main rationale with these MISRA rules is to prevent "compound statement spaghetti" where complex code breaks out from multiple nested compound statements. It's important to understand the rationale before following these rules blindly. So in this case just consider leaving the code as it is - no deviation is needed for advisory rules.

Otherwise, there's a few options as shown below:

One problem with MISRA-C is that it doesn't allow multiple returns from a function, even when it makes the code more readable. Otherwise the obvious and most readable solution would be to use a function:

type do_stuff (void);
{
  type retval;

  retval = do_smth();
  if (retval != OK) { return retval; }

  retval = do_smth2();
  if (retval != OK) { return retval; }

  retval = do_smth3();

  return retval;
}

My usual solution is to make a permanent MISRA-C deviation from the multiple return rule and allow it in cases where it makes the code more readable, like it does in this case.

Otherwise, the 2nd best option might be the old "on error goto" - the rule banning goto was relaxed in MISRA-C:2012 so it's just advisory now.

  retval = do_smth();
  if (retval != OK) { goto error; }

  retval = do_smth2();
  if (retval != OK) { goto error; }

  retval = do_smth3();
  if (retval != OK) { goto error; }

  goto everything_ok;

  error:
    /* error handling */

  everything_ok:

If neither of the above forms are OK because you are super-strict with MISRA-C, then the 3rd option might be something like this, which I believe is 100% MISRA-C compliant:

typedef type do_stuff_t (void);

do_stuff_t* const do_stuff[N] = { do_smth, do_smth2, do_smth3 };
type retval = OK;

for(uint32_t i=0u; (i<N) && (retval==OK); i++)
{
  retval = do_stuff[i]();
}

My point is that do { } while(0u); is not an iteration statement.

The C language disagrees with you.

1) From C17:

6.8.5 Iteration statements

Syntax

iteration-statement:
while ( expression ) statement
do statement while ( expression ) ;

Upvotes: 8

Jabberwocky
Jabberwocky

Reputation: 50831

I'd replace your code with this:

  retval = do_smth();
  if (retval == OK) {
    retval = do_smth2();
  } 
  if (retval == OK) {
    retval = do_smth3();
  }
  • no phony while
  • no gotos disguised as break
  • hence not even a single goto/break
  • hence no more MISRA issues
  • bonus: half the number of lines than in the original code

BTW: the last break (break; // <- 3rd) was useless anyway

Upvotes: 5

Morten Jensen
Morten Jensen

Reputation: 5936

There should be no more than one break or goto statement used to terminate any iteration statement.

Your example has 3 breaks in one do-while (iteration statement), so it is not correct I think. A break is a control-flow/loop-control statement and is only valid in the context of a loop. I don't think your argument is valid here, though I see where you're going.

TL;DR: do-while is still an iteration statement, even though it only runs once.

do {
    retval = do_smth();
    if (retval != OK) {
        break; // <- 1st
    }

    retval = do_smth2();
    if (retval != OK) {
        break;  // <- 2nd
    }
    retval = do_smth3();
    if (retval != OK) {
        break; // <- 3rd
    }
} while (0u);

Upvotes: 4

Related Questions