17

I'm working with a large SDK codebase glommed together from various sources of varying quality / competence / sanity from Linus Torvalds to unidentified Elbonian code slaves.

There are an assortment of styles of code, some clearly better than others, and it's proving an interesting opportunity to expand my knowledge / despair for the future of humanity in alternate measures.

I've just come across a pile of functions which repeatedly use a slightly odd (to me) style, namely:

void do_thing(foo)
{
    do {
        if(this_works(foo) != success)
            break;
        return(yeah_cool);
    } while (0);
    return(failure_shame_death);
}

There's nothing complicated being done in this code (I haven't cut 10,000 lines of wizardry out for this post), they could just as easily do:

if(this_works(foo) == success)
    return(yeah_cool);
else
    return(failure_shame_death);

Which would seem somehow nicer / neater / more intuitive / easier to read.

So I'm now wondering if there is some (good) reason for doing it the other way, or is it just the way they always do it in the Elbonian Code Mines?

Edit: As per the "possible duplicate" links, this code is not pre-processed in any sort of macro, it is just in the normal code. I can believe it might be due to a coding style rule about error checking, as per this answer.

Community
  • 1
  • 1
John U
  • 2,886
  • 3
  • 27
  • 39
  • 2
    Definately Elbonian code slaves. – WW. Oct 17 '13 at 10:44
  • 4
    see this: http://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for – Ferenc Deak Oct 17 '13 at 10:44
  • 2
    Voting to close (duplicate), but I love your writing style. – ixe013 Oct 17 '13 at 10:47
  • Thanks guys - the search hadn't thrown up the duplicates. @ixe013 - thanks! Your cheque is in the post ;) – John U Oct 17 '13 at 10:49
  • You should show actual, completely unedited code, including at least a complete function definition and the preprocessor macro definitions used in it. – Eric Postpischil Oct 17 '13 at 10:50
  • I'd say that the first style may scale better, if you have several checks where the function can fail before returning. The first style allows you to group generic cleanup code into one place, whereas you'd have to replicate that for all early returns in the second style. – sonicwave Oct 17 '13 at 10:50
  • 1
    Building on what sonicwave said, a `goto` would have been more appropriate (everyone thinking I am joking: No, I am not.). – Sebastian Mach Oct 17 '13 at 10:54
  • Clarified above that this code is NOT pre-procesed or in any sort of macro. @EricPostpischil - there's nothing exciting around it, that code is the entire body of (numerous) basic functions. – John U Oct 17 '13 at 10:57
  • 2
    I've used the while loop thingy where there are multiple points within the "loop" where I want to exit the loop and continue in normal flow. Doesn't make sense with just one condition in the loop, other than the programmer may have adopted that style and stuck with it for a bunch of functions, even when the need degenerated in some cases. The "duplicates" talk about handling macros with embedded `;` characters, but the far better solution to that is to write the macro correctly, vs having to "defensively" code for it in dozens of places. – Hot Licks Oct 17 '13 at 12:12

5 Answers5

8

Another guess: maybe you didn't quote the original code correctly? I have seen the same pattern used by people who want to avoid goto: they use a do-while(0) loop which at the end returns a success value. They can also break out of the loop for the error handling:

int doXandY() {
   do {
      if (!x()) {
         break;
     }

     if (!y()) {
         break;
     }
     return 0;
   } while( 0 );

   /* Error handling code goes here. */
   globalErrorFlag = 12345;
   return -1;
}

In your example there's not much point to it because the loop is very short (i.e. just one error case) and the error handling code is just a return, but I suspect that in the real code it can be more complex.

Frerich Raabe
  • 90,689
  • 19
  • 115
  • 207
  • I'm pretty sure they were trying to do this. It's embarrassing how `goto` is such a taboo, that people would go so far just to avoid it, while essentially using it just with another keyword, i.e. `break`. – Shahbaz Oct 17 '13 at 12:36
  • It looks like this answer and Hot Licks comment are closest to the likely answer - some functions do have more than one error case, although I've yet to see any that are complex enough to justify this odd style. @Shahbaz - yeah, goto gets used elsewhere (presumably the Elbonians in the next cave have a different Kung Fu Style) although you're right that it's sometimes quite efficient I can never shake the feeling that _there must be a better way_... – John U Oct 17 '13 at 12:38
  • 1
    @JohnU, actually using `goto` for error handling is both quite common in C and very clean. Look at [this random function I just randomly found](https://github.com/torvalds/linux/blob/master/kernel/cpu.c#L366) in the kernel and imagine what a disaster it would have become to handle errors if `goto` was not used. – Shahbaz Oct 17 '13 at 12:43
  • I use exactly this pattern regularly. I prefer it over `goto out;`, (a common alternative), because once my co-workers see one goto-statement, they tend to add more and more gotos, thinking its okay. Then we end up with spaghetti code. – abelenky Oct 17 '13 at 16:10
  • @Shahbaz One thing which is better about `break` than `goto` is that the former always only jumps downwards in the code, and no further than the current loop. With `goto`, anything can happen and you need to look a lot more carefully. – Frerich Raabe Oct 17 '13 at 22:00
  • @FrerichRaabe, you can also go past an array or access parts of memory that are not yours with pointers, but that doesn't make pointers bad. You could also divide by zero, again, doesn't make division bad. It's the same thing with `goto`. Use it wisely and it's great. Use it badly and it's bad (by definition of "badly"). – Shahbaz Oct 18 '13 at 08:09
  • @Shahbaz Sure, it's exactly the "could" part. With a "break;" there are less things which could happen to the control flow, with a "goto;" anything is possible. So a "break;" is like a more restricted "goto;" in a sense - if it does the job, I'd certainly consider using it. If I see it when reading code, I know that I definately don't have to scroll up to see whether a loop is implemented. – Frerich Raabe Oct 18 '13 at 08:40
  • @FrerichRaabe, that kind of thought was what lead to horrors such as Java. If you see a pointer, do you feel unsafe? Do you think you should scroll up to see if it is properly initialized? Bad programmers write bad code, of course. Let them code in Java. But if a programmer is good enough to be able to code (well) in C, he's good enough to use `goto` in a useful way. I guess what I'm trying to say is that I trust in the intelligence of the programmer, same way gcc does by not implicitly adding `if (b == 0) some_error();` before `a / b`. – Shahbaz Oct 18 '13 at 08:45
  • I think it's quite useless to try to "hide" `goto` by using `break` where the semantics is actually `goto`. If I see `break`, I understand that there is a loop and the loop condition became false in the middle and you need to break out. If I see `goto exit_no_mem`, I understand there is not enough memory and that's an error handling case. If you always use `break` and `do {} while(0)`, you are just removing useful information (such as the meaningful label self-documentating exactly why there's a `goto` and where it goes) and making the code actually counter-intuitive. – Shahbaz Oct 18 '13 at 08:49
  • Last word (I hope you didn't read the comments in an angry way or anything, here, take a smiley embedded in closing parenthesis :), I agree with you that `goto` can lead to spaghetti code and one of your upvotes is mine. I'm just saying `goto` shouldn't be banned because it _can_ be bad. Most of what is there in C _can_ be bad. People just need to learn it's correct use. – Shahbaz Oct 18 '13 at 08:54
6

Some people use the do{} while(0); construct with break; inside the loop to be compliant in some way with MISRA rule 14.7. This rule says that there can be only single enter and exit point in the function. This rule is also required by safety norm ISO26262. Please find an example function:

int32_t MODULE_some_function(bool first_condition,bool second_condition)
{
    int32_t ret = -1 ;

    do
    {
        if(first_condition)
        {
            ret = 0 ;
            break ;
        }

        /* some code here */ 

        if(second_condition)
        {
            ret = 0 ;
            break ;
        }

        /* some code here */ 

    } while(0) ;

    return ret ;
}

Please note however that such a construct as I show above violates different MISRA rule which is rule 14.6. Writing such a code you are going to be compliant with one MISRA rule, and as far as I know people use such a construct as workaround against using multiple returns from function.

In my opinion practical usage of the do{}while(0); construct truely exist in the way you should construct some types of macros.Please check below question, it was very helpful for me :

Why use apparently meaningless do-while and if-else statements in macros?

It's worth notice also that in some cases do{}while(0); construct is going to be completely optimized away if you compile your code with proper optimization option.

Community
  • 1
  • 1
Lazureus
  • 462
  • 4
  • 19
  • 3
    How does that illustrate the point? You could drop the `do {` and `} while (0);` and it would behave the same way. Using `do { ... } while (0);` like this doesn't make any sense unless there's a `break;` statement inside the loop. – Keith Thompson Oct 17 '13 at 15:24
  • That's true, as I said some people use this as a workaround to fulfill single enter/exit point function. If you don't use `do{}while(0);` construct, then probably other idea would be to use nested if's. This will be even more compliant with MISRA I'd say, but in my opinion it's harder to read. Personally I'm supporter of the multiple returns from function and honestly don't understand rationale behind single exit point from function. – Lazureus Oct 17 '13 at 15:33
  • 2
    I understand the point. What I'm saying is that your example doesn't illustrate that point. The declaration and use of `ret` is what avoids multiple exits; the do/while, in your example, is nothing more than noise. A valid example would have one or more `break;` statements inside the do/while. – Keith Thompson Oct 17 '13 at 15:37
  • Yes, that's true,I forgot about breaks actually. Thanks for noticing it. Now it should be better. – Lazureus Oct 17 '13 at 15:44
  • You can have a single-entry-single-exit function with `goto` just as well, I don't understand how using `do-while(0)` is helpful here. – Frerich Raabe Oct 18 '13 at 07:10
  • Yes you can, however `goto` is considered as harmful,and as far as I know `goto` seems to be helpful only in case error handling to make some forward jumping. – Lazureus Oct 18 '13 at 08:00
5

Hm, the code might be preprocessed somehow. The do { } while(0) is a trick used in preprocessor macros; you can define them like this:

#define some_macro(a) do { whatever(); } while(0)

The advantage being that you can use them anywhere, because it is allowed to put a semicolon after the while(0), like in your code above.

The reason for this is that if you write

#define some_macro(a) { whatever(); }

if (some_condition)
    some_macro(123);
else
    printf("this can cause problems\n");

Since there is an extra semicolon before the else statement, this code is invalid. The do { ... } while(0) will work anywhere.

Johan Henriksson
  • 687
  • 3
  • 10
1

do {...} while(0) arranged with "break" is some kind of "RAII for Plain C".

Here, "break" is treated as abnormal scope exit (kind of "Plain C exceptions"), so you can be sure that there is only one place to deallocate a resource: after a "while(0)". It seems slightly unusual, but actually it's very common idiom in the world of plain C.

Yury Schkatula
  • 5,291
  • 2
  • 18
  • 42
0

I would guess that this code was originally written with gotos for error handling:

void do_thing(foo)
{
    if(this_works(foo) != success)
        goto error;
    return(yeah_cool);
error:
    return(failure_shame_death);
}

But at some point an edict came down from on high "thou shalt not use goto", so someone did a semi-automatic translation from goto style to loop-break style (perhaps with simple script). Probably when the code was merged/moved from one project to another.

Chris Dodd
  • 119,907
  • 13
  • 134
  • 226