19

I found the following code in a C program:

while (1)
{
    do_something();
    if (was_an_error()) break;

     do_something_else();
     if (was_an_error()) break;

     [...]

     break;
}
[cleanup code]

Here while(1) is used as local emulation of "finally". You can also write this using gotos:

do_something()
if (was_an_error()) goto out;

do_something_else()
if (was_an_error()) goto out;

[...]
out:
[cleanup code]

I thought the goto solution is a usual idiom. I have seen several occurrences of this idiom in the kernel sources and it is also mentioned in Diomidis Spinellis' "Code Reading" book.

My question is: What solution is better? Is there any specific reason to use the while(1) solution?

Question 943826 doesn't answer my question.

Community
  • 1
  • 1
dmeister
  • 34,704
  • 19
  • 73
  • 95
  • 24
    Using while(1) for this makes whoever reads the code expect it to run "infinitely"... Why not use do { } while(0) ? – Tal Pressman Jul 02 '09 at 09:39
  • 1
    @Tal Pressman - why would that be any different? – Daniel Earwicker Jul 02 '09 at 09:40
  • 2
    @Earwicker, with that option it is obvious that it will run at least once, and that it won't run again because the condition is "while (0)". The confusing "while(1)" does not appear. – Daniel Daranas Jul 02 '09 at 09:44
  • @Earwicker: the reader of while(1) thinks it is an infinite loop. The reader of do presumes it is finite. best for coherence might be for (int dummy = 0; dummy < 1; dummy++) – yairchu Jul 02 '09 at 09:49
  • 2
    But both use a looping structure where there is never any intention of looping. Both are misleading, obscure, etc. – Daniel Earwicker Jul 02 '09 at 10:49
  • `goto`s in kernel are idioms, yes. But linux kernel has its own code standards, that might or might not be applicable to your case. I'd at least use do...while(0); instead of your loop. – liori Jul 02 '09 at 11:00
  • 1
    @Earwicker: I agree, but one is even worse in that it suggests an infinite loop. However, using loops for things that are not loops in the first place is a clear sign of bad design. – Daniel Daranas Jul 02 '09 at 11:25

13 Answers13

36

The seemingly universal revultion to GOTO is largely due to Edsger Dijkstra's letter "Go To Statement Considered Harmful".

If you are determined not to use goto, something like

do {
    ...
while(0);

is probably safer than while(1) { ... } since it guarantees that you will not inadvertently loop (and if you are inadvertently looping, with while(1) you are probably inadvertently looping infinitely).

The one advantage that (ab)using do/break/while or while/break for this purpose has over goto is that you are guaranteed not to be jumping above the construct -- goto can be used to jump to a label earlier within the same function.

The disadvantage that do/break/while etc. have over goto is that you are limited to one exit point (immediately after the loop). In some cases you might need a staged cleanup: e.g., when you open a file handle, malloc some memory, read from the file... if the read fails, you need to clean up the malloc. If the malloc fails, you don't need to clean it up, but you still need to clean up the file handle. With goto, you can have one label per stage of cleanup and jump to precisely the right point depending on where your error occurred.

In my opinion blindly avoiding GOTO because of the prevalent hatred of it is more damaging than carefully reasoning out a case for its use on a case-by-case basis. A rule of thumb I use is "does the Linux kernel do it? If so, it can't be that bad". Substitute linux kernel with any other good example of modern software engineering.

jmtd
  • 1,223
  • 9
  • 11
  • 3
    Actually it's not only Linux kernel, Windows Drivers samples from MS full of goto's, Solaris unix kernel drivers use it as well, BSD kernel code, etc ... The bottom line goto is used and used widely mostly by people knowing what they are doing. – Ilya Jul 02 '09 at 15:36
  • 6
    I've seen code using the `do { ... break ... } while(0);` trick, and I can attest that it hampers readability while not buying you anything. If you're doing a `goto` - and you _are_ doing it here anyway - then at least be explicit about it. And there's nothing wrong about `goto` in C code for error handling / cleanup purposes. – Pavel Minaev Jul 08 '09 at 23:40
  • 1
    +1 but I almost didn't because of the last 2 sentences. Linux kernel is full of some really bad coding practices... – R.. GitHub STOP HELPING ICE Apr 24 '11 at 21:19
10

Putting the code into a separate function, and using return to exit early is another way to do it, with the benefit of easy integration of a return code indicating the nature of the failure.

Barry Kelly
  • 41,404
  • 5
  • 117
  • 189
  • 1
    The problem is not early exit, it is cleanup. In a multistep init, you eventually need to cleanup / deallocate what was already successfully allocated. – shodanex Jul 02 '09 at 09:50
  • 1
    This is what I was going to suggest. Failing that, I would use goto over while(1), because while(1) gives the misleading impression that the code loops, when in fact it doesn't. – Zebra North Jul 02 '09 at 09:52
  • 1
    @shodanex: it might just be the case that the language is not expressive enough to natively express what you wanted to do (that is, a "finally" clause like in java). so any kind of hacks to get around that limitation is going to have some cons associated with it. – Chii Jul 02 '09 at 09:53
  • A method call may have performance impacts caused by increasing the stack as well as possible stack overflow exceptions. This is one of the reasons you see them in drivers and kernel code. – Matthew Whited Dec 09 '09 at 17:53
  • The compiler will inline the method call if it makes sense to, therefore negating the performance penalty. – jmh Oct 19 '12 at 23:16
8

I know my style isn't the coolest possible, but I prefer it because it doesn't need any special constructs and is concise and not too hard to understand:

error = (!error) && do_something1();
error = (!error) && do_something2();
error = (!error) && do_something3();

// Cleanup code
Makis
  • 12,468
  • 10
  • 62
  • 71
  • 1
    if `do_something1()` returns `1` then 2nd line changes `error` to `0` again and `do_something3()` is executed. It might be not what you want. – jfs May 07 '10 at 18:15
  • For bonus points on hideousness, `#define FOO error+=!error&&` and then `FOO do_something1();` ... ;-) – R.. GitHub STOP HELPING ICE Apr 24 '11 at 21:23
  • 1
    Even with `+=`, you can't determine which `do_something` caused errors this way, only the number of ones that did. – jmtd May 27 '11 at 08:30
  • This has the same principle but is a little cleaner: `error = do_something1() && do_something2() && do_something3()`. Put the do_somethings on separate lines for readability (can't figure out how to do that in a comment). – presto8 Feb 04 '12 at 06:03
7

Though the use of goto is discouraged usually, some rare situations like yours is a place where best-practices are not the best.

So, if goto makes the clearest code I would use it. using a while(true) loop to emulate goto is something unnatural. What you really need is a goto!

Khaled Alshaya
  • 94,250
  • 39
  • 176
  • 234
  • 2
    +1 Use of GOTO is common in embedded systems to clean up after a failure. – g . Jul 02 '09 at 09:57
  • 2
    +1 Use of goto is not discouraged "just because". It's discouraged to avoid spaghetti code and to enhance clarity. The real rule is that the function should be clear and simple, so using an even harder to grasp alternative for goto isn't a good option IMO. Anything involving once-only loops would fall into this category. – Daniel Daranas Jul 02 '09 at 10:00
5

Why not use a series of if statements? I usually write it this way, as I find it much clearer than a loop:

bool ok = true;

do_something();
if (was_an_error()) ok = false;

if (ok)
{
    do_something_else();
    if (was_an_error()) ok = false;
}

if (ok)
{
    do_something_else_again();
    if (was_an_error()) ok = false;
}

[...]

[Cleanup code]

Also if you are working to strict coding standards, yes goto is likely to be forbidden, but often so are break and continue so the loop is not necessarily a workaround for that.

finnw
  • 47,861
  • 24
  • 143
  • 221
  • 1
    That can be compressed a little: if(ok) {do_something(); ok = was_an_error();} – Steve Melnikoff Jul 02 '09 at 12:27
  • In a real-time system this could have a performance penalty. You are doing several conditional jumps instead of just one. – Matthew Whited Dec 09 '09 at 17:55
  • 1
    @Matthew, an optimising compiler can spot that if one `if (ok)` test fails, then so will all the remaining tests, so it can jump directly to the exit point. – finnw Dec 09 '09 at 19:19
5

"break" understands the semantics of the block scope, while "goto" is oblivious to it. In other words, "while-break" can be translated into functional languages like Lisp with tail-recursion, "goto" cannot.

yogman
  • 4,021
  • 4
  • 24
  • 27
3

Normally, GOTOs are considered bad but at some places where there are only Forward Jumps through GOTOs, they are not AS bad. People avoid GOTO like plague but a well-thought-out use of GOTO is sometimes a better solution IMHO.

Aamir
  • 14,882
  • 6
  • 45
  • 69
2

I think that this use (for resource management) of goto is ok.

dfa
  • 114,442
  • 31
  • 189
  • 228
2

Use it if you can't use goto for whatever reason

  • forbidden in your project's conventions
  • forbidden by your lint tool

I also think that is also one of the cases where macros aren't evil:

#define DO_ONCE for (int _once_dummy = 0; _once_dummy < 1; _once_dummy++)
yairchu
  • 23,680
  • 7
  • 69
  • 109
2

"do while" and "goto out" are different on these area:

1.local variable initialization

void foo(bool t = false)
{
    if (t)
    {
        goto DONE;
    }

    int a = 10; // error : Goto bypass local variable's initialization 

    cout << "a=" << a << "\n";
DONE:
}

It is fine to initialize in-place local variables in do ... while(0) block. 

void bar(bool t = false)
{
    do{
        if (t)
        {
            break; 
        }

        int a = 10;  // fine

        cout << "a=" << a << "\n";
    } while (0);

}

2 difference for Macros. "do while" is a slight better. "goto DONE" in a Macro is so not the case. If the exit code is more complicated, let see like this:

err = some_func(...);
if (err)
{
    register_err(err, __LINE__, __FUNC__);
#if defined (_DEBUG)
    do_some_debug(err)
#endif
    break;
}

and you write this code again and again, you will probably put them into a Macro. 

#define QUIT_IF(err)                     \
if (err)                                       \
{                                              \
    register_err(err, __LINE__, __FUNC__);     \
    DO_SOME_DEBUG(err)                         \
    break; // awful to put break in macro, but even worse to put "goto DONE" in macro.  \
}

And the code become:

do
{
    initial();

    do 
    {
        err = do_step1();
        QUIT_IF(err);

        err = do_step2();
        QUIT_IF(err);

        err = do_step3();
        QUIT_IF(err);

        ....
    } while (0);
    if (err) {     // harder for "goto DONE" to get here while still using macro.
        err = do_something_else();
    }
    QUIT_IF(err);
    .....
} while (0);

3.do... while(0) handles different levels of exiting with same macro. Code is shown above. goto ... is not the case for Macro cause you need different labels for different levels.

By saying that, I do not like both of them. I'd prefer to use the exception method. If exception is not allowed, then I use "do ... while(0)", since the whole block is indented, it is actually easier to read than "goto DONE" style.

Jack Wu
  • 174
  • 1
  • 3
1

I like the while(1) approach. I use it myself. Especially, when the loop might get repeated by continue, e.g. when an element is processed inside such loop, and it's done in multiple approaches.

1

Never use a condition loop with a permanently true condition. Since the condition is always true, why use a conditional loop?

Permanently true conditions are most directly represented by a goto.

  • 1
    I find while(1)+break+continue perfectly readable for me.. in opposition to goto, which is IMO unreadable. –  Jul 02 '09 at 09:45
  • @emg-2 I can read both. But "while(1)" communicates me a false idea of an infinite loop (which I have seen in other situations) and not a simple function which runs only once. "goto" is better for that purpose. All functions are either a sequence, a loop or a conditional. – Daniel Daranas Jul 02 '09 at 09:47
  • So you're saying that an application's message loop would be better represented with a goto to the beginning of the message handler instead of a while(1) or for(;;) ? – Tal Pressman Jul 02 '09 at 09:47
  • What he's saying is, the loop should have a terminating condition. Especially if -- as is the case here -- you are not intending to loop at all. With a message handling loop you do actually intend to leave it at some point, right? So why not build the terminating condition into the loop conditional? – jmtd Jul 02 '09 at 10:20
  • @emg-2: the readers ability to easily read or not easily read code does not relate to the semantic meaning of that code - here you are arguing for the use of semantically inappropriate code on the basis of readability - which is contradictory –  Jul 02 '09 at 13:57
  • @Tal: yes, absolutely. Would you use a goto with an if() inside which triggers a break, instead of using a while() with the condition? –  Jul 02 '09 at 13:59
  • @Tal: jtmd is correct - assuming no exit condition in the loop - if there is an exit condition, then (of course) you use a while(). –  Jul 02 '09 at 14:00
  • I would find a "do{}while(0);" loop most idiomatic here myself, as the goal is to establish a scoping block but not actually loop. If the goal were to actually loop, I would generally use "do{}while(1);" although there are some conditions where a loop can early-exit (e.g. found what it's looking for), repeat (e.g. need to look more), or fall-through (all data searched without finding target). In that case, a "do{}while(0);" with break and continue can be handy. – supercat Dec 06 '11 at 18:53
0

While using "goto" for error handling situations is fairly common, I'd still prefer the "while" solution (or "do while"). In the "goto" case, there are far fewer things that the compiler can guarantee. If you make a typo in the label name, the compiler can't help you there. If someone uses another goto to another label in that block, there's a good chance the cleanup code won't get called. When you use the more structured flow control constructs you are always guaranteed which code will run once the loop is over.

Tal Pressman
  • 7,199
  • 2
  • 30
  • 33
  • The compiler can catch label typos easily: temp.c:9: error: label ‘blarg4h’ used but not defined temp.c:7: warning: label ‘blargh’ defined but not used – jmtd Jul 02 '09 at 10:24