23

The goto statement has been examined at great length in several SO discussions (see this and that), and I certainly don't want to revive those heated debates.

Instead, I'd like to concentrate on a single use case of gotos and discuss its value and possible alternatives.

Consider the following code snippet, which is common in (at least my own) FSMs:

while (state = next_state()) {
        switch (state) {
                case foo:
                        /* handle foo, and finally: */
                        if (error) goto cleanup;
                        break;
                case bar:
                        /* handle bar, and finally: */
                        if (error) goto cleanup;
                        break;
                /* ...other cases... */
        }
}

return ok;

cleanup:
/* do some cleanup, i.e. free() local heap requests, adjust global state, and then: */
return error;

Swapping out the cleanup stuff in a separate function just in order to save the gotos seems awkward. On the other hand, we've been raised to condemn the use of gotos wherever possible.

My question: is my code example considered good style?
If not, are there feasible alternatives available?

Please adhere to the specific usage of goto described above. I don't want to delve into yet another discussion about the general use of goto.

user694733
  • 15,208
  • 2
  • 42
  • 68
Philip
  • 5,795
  • 3
  • 33
  • 68
  • 3
    I'd like you see the cleanup stuff in a seperate function without passing lots of arguments that are just local variables that would be readily available for the goto solution. –  Apr 18 '11 at 11:31
  • I guessed that, I just think it's better to spell it out. Might save us one or two answers that didn't think of this. –  Apr 18 '11 at 11:34
  • also see http://stackoverflow.com/questions/788903/valid-use-of-goto-for-error-management-in-c – Alex K. Apr 18 '11 at 11:36
  • 4
    "we've been raised to condemn the use of gotos wherever possible" - well there's your problem ;-p – Steve Jessop Apr 18 '11 at 11:40
  • 2
    @philip - Yes, "we've been raised to condemn the use of gotos wherever possible." But there are plenty of places (as in your code above) where goto is ***reasonable.*** To me, gotos that eliminate big, deeply-nested blocks are reasonable because they improve the readability of the code, particularly where the gotos all point to the same label. The reader quickly understands and appreciates this relatively rare use where goto might ***get rid of*** spaghetti. – Pete Wilson Apr 18 '11 at 11:50
  • 1
    @Pete Wilson: I absolutely agree. There are several problems where careful use of goto is the cleanest, most idiomatic solution. The two I encounter most often is this one, where I need to do resource cleanup, and also efficient implementation of state machines. Unfortunately, goto is now so unfashionable that it's practically impossible to have a reasonable discussion of it, and most new HLLs don't support it in any form. Just try asking about it in a Javascript forum some time. Bring your asbestos underwear. – David Given Apr 18 '11 at 12:46
  • @David: hi, David :-) We spent way too many years writing assembly to be anything like as phobic of goto as you're "supposed" to be... – Steve Jessop Apr 18 '11 at 17:24
  • +1 for the question's title (actually, I'd +1 for the question as well, but I only get one upvote). – Michael Burr May 15 '11 at 02:26
  • I've been writing C programs for 15 years and I have never had the need to use goto, or continue. Whenever I have felt such a need, I have simply looked at my program design and found flaws in it. That being said, I believe goto is one justified way to break out of multiple nested loops, _if it makes the code more readable_. Same thing with multiple returns in a function: it can be justified, but only if it makes the code more readable. Generally, for some reason programmers think returns are easier to read, so I'd rather put such nested loops inside a function and return upon error. – Lundin May 17 '11 at 09:38

10 Answers10

11

Your usage of goto is ok. It doesn't break the 2 good ways to use goto.

  1. gotos MUST go down (a few lines) in the source
  2. The innermost block of goto labels MUST contain the goto statements
pmg
  • 106,608
  • 13
  • 126
  • 198
  • 3
    Would you automatically condemn `goto` that goes "up" in a function? I've had several situations where a rare corner case had to restart a complex function from the top, and adding a single label and goto was a lot less obtrusive than restructuring the whole function as a loop would have been. Surely I could have just had the function call itself, but I generally consider recursion (and relying on the compiler's tail-call optimize to remove it) a worse offense than `goto`. – R.. GitHub STOP HELPING ICE Apr 18 '11 at 23:42
  • 3
    I don't automatically condemn anything that goes against "the rules" and I don't automatically accept anything that goes by "the rules" :) But, when there are bugs and no indication of where to find then, code that goes against the rules is scrutinized first. – pmg Apr 19 '11 at 00:06
  • No it is not ok, it is _completely redundant_, as shown in my answer below. There are no bonus points handed out for each C keyword in your program. Just because C has lots of strange, redundant features doesn't mean you must use them. – Lundin May 17 '11 at 09:24
  • 1
    @Lundin I don't see your answer. Did you delete it? – Kyle Strand Sep 10 '14 at 18:41
  • And @pmg, what does your second point mean? Just that you shouldn't use `GOTO` to jump in and out of blocks? – Kyle Strand Sep 10 '14 at 18:42
  • @KyleStrand: basically yes. You shouldn't use `goto` to jump *into* blocks, but you can use it to jump out of blocks. – pmg Sep 10 '14 at 19:10
7

Instead of extracting the cleanup logic into its own function and calling that from different places, I would consider extracting the switch statement into a separate function and returning an error code from that. In your while loop you could the check the return code and do the cleanup and return if neccessary.

If you have several resources shared between the switch and cleanup logic then I think the goto would be preferrable to passing all this state around.

Jörn Horstmann
  • 33,639
  • 11
  • 75
  • 118
  • 2
    örn: That's an intriguing solution, thank you very much for pointing this out. It suffers from the same issue as the separate cleanup function in that it becomes necessary to pass (possibly lots of) somewhat "local" resources around, but for simple setups, this seems like the way to go. – Philip Apr 18 '11 at 12:42
5

I have seen goto used in this manner in the OpenBSD kernel, particulary within ATA device drivers (one such example) and I personally feel that it is good style, as it helps illustrate exactly what is happening and how the code matches up to the corresponding FSM. When trying to verify functionality of an FSM against a specification, this use of goto improves clarity somewhat.

mdec
  • 5,122
  • 4
  • 25
  • 26
  • This is a quite different style of goto usage, there is no state machine involved. This is a "do a bunch of init and use goto for error" type of code – shodanex Apr 18 '11 at 11:55
  • Ahh, I linked to the wrong file =] [this one](http://www.openbsd.org/cgi-bin/cvsweb/src/sys/dev/ata/wd.c?rev=1.100;content-type=text%2Fx-cvsweb-markup) uses a FSM to implement the ATA FSM in certain parts. – mdec Apr 18 '11 at 11:58
5

Goto is not needed when you have switch. Using both switch and goto just adds complication.

while (state) {
        switch (state) {
                case cleanup:
                        /* do some cleanup, i.e. free() local heap requests, adjust global state, and then: */
                        return error;
                case foo:
                        /* handle foo, and finally: */
                        if (error) { state = cleanup; continue; }
                        break;
                case bar:
                        /* handle bar, and finally: */
                        if (error) { state = cleanup; continue; }
                        break;
                /* ...other cases... */
        }
        state = next_state();
}

return ok;
Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • 2
    Putting the cleanup inside the state machine is a nice improvment. Apart from that, I'm pretty sure there is scientific research proving that continue == goto with another flavour. What your code does is to jump up instead of down, continue instead of goto, that's no improvment at all, it is equal spaghetti. Just dogmatically saying that goto is bad because it is named goto isn't helping your code. – Lundin May 17 '11 at 09:30
  • @Lundin: Well, I don't like the separation of `next_state` into a separate function either. It belongs with the per-state logic in this switch block. Fix that, and the `continue` goes away as well, you just have `state = next_state;` or `state = cleanup;` and either one is followed by `break;` at the end of the case block. – Ben Voigt May 17 '11 at 12:35
  • I agree. To let some other function that is unaware of the current state decide the next, is strange program design. – Lundin May 17 '11 at 13:31
  • 1
    In what way is "state = something; continue;" superior to or clearer than a `goto`? Given the statement "goto CLEANUP;" one can ascertain what it does simply by finding the label "CLEANUP". By contrast, if code sets `state` and then performs a `continue`, it's necessary to examine more code. Things get even worse if the state of the object when performing cleanup code might not always be the same. – supercat May 13 '12 at 20:46
  • 1
    @supercat: Finite State Machines are a very well understood design technique, with computer aided design tools for detecting stuck states, unreachable states, etc. – Ben Voigt May 13 '12 at 21:55
  • This sounds ingenious at first, but on deeper thought, it gives me the shivers: The cleanup code ends up precisely in the last place that I would expect, right in the middle of the meat of the function. The `goto` approach at least puts everything precisely at the place where it belongs. – cmaster - reinstate monica Aug 01 '14 at 15:09
  • @cmaster: If you're using a state machine, the state logic is where *everything* belongs. Besides, using cleanup states enables other patterns such as non-blocking cleanup (spread out over multiple states). – Ben Voigt Aug 01 '14 at 15:22
  • 1
    @cmaster: Something else you might have missed is that the cleanup state elegantly allows retries. After cleanup you can (depending on your retries-remaining counter) transition back to the initial state and try again. – Ben Voigt Aug 01 '14 at 15:26
4

I'd say if the cleanup code can't be generalized, i.e. it's specific for the function it's being used in, the goto is a nice and clean way to do it.

das_weezul
  • 6,082
  • 2
  • 28
  • 33
  • 1
    I have some beef about the whole writing a whole new function just to return the result of instead of using a very simple goto, simply because it doesn't exist. – Claudia Sep 11 '14 at 04:21
2

Looking at Ben Voigt's answer gave me an alternative answer:

while (state = next_state()) {
        switch (state) {
                case foo:
                        /* handle foo, and finally: */
                        /* error is set but not bothered with here */ 
                        break;
                case bar:
                        /* handle bar, and finally: */
                        /* error is set but not bothered with here */
                        break;
                /* ...other cases... */
        }

        if (error) {
                /* do some cleanup, i.e. free() local heap requests, */
                /* adjust global state, and then: */
                return error;
        }
}

return ok;

The downside of this is that you have to remember, after it process a state, it might clean up if there's an error. It looks like the if structure could be an if-else chain to handle different types of errors.

I haven't taken a formal class on FSMs, but it looks to me like the code you posted has the same behavior.

Prashant Kumar
  • 20,069
  • 14
  • 47
  • 63
  • That will replicate the cleanup code between the `if()` body and the part after the loop. But your answer shares this shortcoming with Ben Voigt's answer. – cmaster - reinstate monica Aug 01 '14 at 15:12
  • @cmaster: If, using my method, you have cleanup code outside the `switch` then you have a bug in your state transitions. Cleanup is a state (not unique in any way), it's not just a magic way of cramming unique code into an FSM. – Ben Voigt Aug 01 '14 at 15:24
  • I agree with Ben Voigt here, cleanup is a state requiring no more than what the FSM framework already provides. Cleanup in my code is handled in a needlessly special way, but I think this construction feeds the "no `goto`/`continue`" purists. – Prashant Kumar Aug 01 '14 at 17:33
  • 1
    Adding a flag to a method is semantically equivalent to making two copies of the code in the method--one where tests always behave as if the flag is "false", and one where it's always "true". Setting or clearing the flag is equivalent to doing a "goto" from one part of the code into the corresponding location in the other part. In cases where not using the flag would necessitate duplicating code, using the flag is likely better. But in cases where using a `goto` could eliminate the flag without requiring code duplication, the `goto` is often better. – supercat Dec 13 '14 at 20:41
1

A general principle I like to follow is that one should when possible try to write code whose flow and design fits that of the problem domain ("what the program needs to do"). Programming languages include control structures and other features which are good fits for most, but not all, problem domains. Such structures should be used when they match the program's requirements. In cases where a program's requirements are not a good match for a language's features, I prefer to focus on writing code which expresses what the program needs to do, than to contort the code to fit patterns which, while they meet the requirements of other applications, don't really meet the requirements for the program being written.

In some cases, a very natural way of translating a state machines into code, in cases where a routine will be able to not have to "exit" until a state machine has run to some form of conclusion, is to have a goto label represent each state, and use use if and goto statements for the state transitions. If the required state transitions would be a better fit for other control structures (e.g. while loops), using such loops would be better than goto statements, and using switch statements may make certain kinds of "adaptations" (e.g. having a routine perform state transition each time it's executed, rather than requiring it to immediately run to completion) much easier. On the other hand, since a switch statement is really just a "goto" in disguise, it may be cleaner to simply use a goto directly than use a switch statement to imitate one.

supercat
  • 77,689
  • 9
  • 166
  • 211
1

If all your init code is done before entering the while loop, then your gotos are useless, you can do the cleanup when exiting the loop. If your state machine is all about bringing up stuff in the correct order, then why not, but since you have a state machine, why not use it to do the cleanups ?

I am not against goto when initializing several thing together, and having a simple error handling code, as discussed here. But if you go through the trouble of setting up a state machine, then I can't see a good reason to use them. IMO, the question is still too general, a more practical example of state machine would be useful.

Community
  • 1
  • 1
shodanex
  • 14,975
  • 11
  • 57
  • 91
  • 1
    AFAIKS the advantage of the `goto` here is that you can "`break`" out of the `switch` and the `while` at the same time. If you'd just update a state variable, `break` out of the `switch` and the test again to `break` the `while` you easily will spaghetti your code. – Jens Gustedt Apr 18 '11 at 12:26
1

The use of goto to cleanup the code by breaking multiple nesting for loop is very convenient, instead of setting flags and checking it in each nesting. For example, if you are unable to open a file and you discover it deep in a nesting, you can simply goto the cleanup segment and close the files and free resources. You can see such goto examples in the coreutilities tools sources.

dns
  • 2,753
  • 1
  • 26
  • 33
phoxis
  • 60,131
  • 14
  • 81
  • 117
0

If you just need some cleanup code to be able to be called from multiple places in your procedure and it needs to access local resources, maybe use a statement lambda instead. Define it before your switch logic and just call it where you need to clean up. I like the idea for a couple reasons: 1) It's cooler than a goto (and this is always important) 2) You get the clean encapsulation of logic without having to create an external method and pass a bunch of parameters since the lambda can access the same local variables withing the closure.

Kevin Buchan
  • 2,790
  • 3
  • 27
  • 34
  • 1
    I should have been more specific. Although there's the "C" tag, I didn't mention the target language in my posting. (Un)fortunately, C doesn't natively support lambda expressions. Your idea could be implemented with macros, but in this case I'd rather stick with `goto`. – Philip Apr 18 '11 at 13:04
  • Ummm... sorry. I feel a little foolish. – Kevin Buchan Apr 20 '11 at 11:46