acceptable use of goto?

[myheartinamerica]

Hello,

I was doing some simple database programming and encapsulating
different
queries in functions. After each library call, I check the return
value. After
some return values, it makes no sense to continue, so I returned an
error code.
Pseudocode is something like this:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) { return -1}
retval = library_call_2()
if (retval == ERROR) { return -1}
retval = library_call_3()
if (retval == ERROR) { return -1}
/* process data */
/* cleanup handles, allocations, etc... */
return some_inportant_value

When I came in this morning, a coworker had rewritten my code. I'm
quick second
guess myself, so I assumed that his was more correct. It was something
like:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
retval = library_call_2()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
retval = library_call_3()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
/* process data */
/* cleanup handles, allocations, etc... */
return some_inportant_value

So, he would continue to make the other library calls, even if the
previous
ones had failed (meaning that the later ones couldn't be successful).

It got me thinking, that maybe there was a better way to do this, but
I'm not
sure what that is. I'm thinking about using goto's and a label, like
this:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_2()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_3()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
/* process data */
clean_up:
/* cleanup handles, allocations, etc... */
return some_important_value


Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

 

[myheartinamerica]

I found an example of someone writing some code for NetBSD in a
similar situation (perhaps my explanations weren't very clear).

http://julipedia.blogspot.com/2005/08/using-gotos-in-c.html

 

[Willem]

myheartinamerica wrote:
) Hello,
)
) I was doing some simple database programming and encapsulating
) different
) queries in functions. After each library call, I check the return
) value. After
) some return values, it makes no sense to continue, so I returned an
) error code.
) Pseudocode is something like this:
)
) int query_customer(const char* customer)
) int retval = library_call_1()
) if (retval == ERROR) { return -1}
) retval = library_call_2()
) if (retval == ERROR) { return -1}
) retval = library_call_3()
) if (retval == ERROR) { return -1}
) /* process data */
) /* cleanup handles, allocations, etc... */
) return some_inportant_value
)
) When I came in this morning, a coworker had rewritten my code. I'm
) quick second
) guess myself, so I assumed that his was more correct. It was something
) like:
)
) int query_customer(const char* customer)
) int retval = library_call_1()
) if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
) retval = library_call_2()
) if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
) retval = library_call_3()
) if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
) /* process data */
) /* cleanup handles, allocations, etc... */
) return some_inportant_value
)
) So, he would continue to make the other library calls, even if the
) previous
) ones had failed (meaning that the later ones couldn't be successful).

Your cow orker suffers from rigid adherence to coding standards.
He read somewhere 'there should be only one exit point from a function'
and made these silly changes.

By the way, are you sure he didn't have elses in there somewhere,
preventing the calls to library_call_2 and _3 ?

) It got me thinking, that maybe there was a better way to do this, but
) I'm not
) sure what that is. I'm thinking about using goto's and a label, like
) this:
)
) int query_customer(const char* customer)
) int retval = library_call_1()
) if (retval == ERROR) {
) some_important_value = MY_ERROR_CODE;
) goto clean_up;
) }
) retval = library_call_2()
) if (retval == ERROR) {
) some_important_value = MY_ERROR_CODE;
) goto clean_up;
) }
) retval = library_call_3()
) if (retval == ERROR) {
) some_important_value = MY_ERROR_CODE;
) goto clean_up;
) }
) /* process data */
) clean_up:
) /* cleanup handles, allocations, etc... */
) return some_important_value
)
)
) Is there some better way to do this? Is the use of goto a good example
) of it's
) usefulness?

No, the original code with multiple returns is better than the use of
goto's.

However, what you 'should'(*) do is:


int query_customer(const char* customer) {
some_important_value = MY_ERROR_CODE;
if ((library_call_1() != ERROR)
&& (library_call_2() != ERROR)
&& (library_call_3() != ERROR)) {
/* process data */
some_important_value = MY_OK_CODE;
}
/* cleanup handles, allocations, etc... */
return some_important_value
}

Or something similar.


*) This, of course, depends on the function calls being clean like that.
When faced with more complicated calls, you can use nested 'if's.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[myheartinamerica]

*) This, of course, depends on the function calls being clean like that.

When faced with more complicated calls, you can use nested 'if's.

In this situation, the calls are indeed more complex. Nested if
statements, while producing the correct output, would be obnoxiously
nested at 4+ levels deep. I've thought about that solution many times,
but stylistically felt like it was the wrong solution, even more so
than using the goto statement.

 

[Dave Hansen]

Hello,

I was doing some simple database programming and encapsulating
different
queries in functions. After each library call, I check the return
value. After
some return values, it makes no sense to continue, so I returned an
error code.
Pseudocode is something like this: [...]
Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

A post of mine from a couple years ago in a different group:
http://groups.google.com/group/comp.arch.embedded/msg/3534b92281dc0f81?dmode=source

(watch wrap)

Summary: "goto" can be useful for error recovery in a manner similar
to that you describe above. Like most other features of C, it can be
harmful if misused. However, it is one of the easiest features of C
to misuse...

HTH,

-=Dave

 

[William Pursell]

myheartinamerica wrote:

) Pseudocode is something like this:
)
) int query_customer(const char* customer)
) int retval = library_call_1()
) if (retval == ERROR) { return -1}

) When I came in this morning, a coworker had rewritten my code

) something like:
)
) int query_customer(const char* customer)
) int retval = library_call_1()
) if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
) retval = library_call_2()

) So, he would continue to make the other library calls, even if the
) previous
) ones had failed (meaning that the later ones couldn't be successful).

This is almost certainly incorrect.

) It got me thinking, that maybe there was a better way to do this, but
) I'm not
) sure what that is. I'm thinking about using goto's and a label

No, the original code with multiple returns is better than the use of
goto's.

I disagree. Note that this disagreement is purely stylistic,
and you'll have to decide on your own. Personally, I
prefer to do this sort of thing as:

int status = 0;

if( ! status )
status = library_call1();
if( ! status )
status = library_call2();
....
(Assuming zero indicates success for the library calls.)

However, what you 'should'(*) do is:

int query_customer(const char* customer) {
some_important_value = MY_ERROR_CODE;
if ((library_call_1() != ERROR)
&& (library_call_2() != ERROR)
&& (library_call_3() != ERROR)) {
/* process data */
some_important_value = MY_OK_CODE;
}
/* cleanup handles, allocations, etc... */
return some_important_value
}

Or something similar.

This style is tempting to use, but it's annoying
when using a debugger, because it is impossible
to tell which library call fails. It's also
somewhat more difficult to modify. An alternative
is:
if ( 1
&& (library_call_1() != ERROR)
&& (library_call_2() != ERROR)
&& (library_call_3() != ERROR)
) {

OR
if (
(library_call_1() != ERROR) &&
(library_call_2() != ERROR) &&
(library_call_3() != ERROR) &&
1
) {

This is a trivial modification that allows conditions
to be added/removed in a uniform way, but in the
long run it really is easier to deal with the
slightly more verbose looking code farther above.

Basically, it's purely a matter of stylistic
preference, except that your coworkers modifications
which call the subsequent library functions without
regard to previous failures is almost certainly in
error. For example, if all of the calls fail except
the last, your function will return a successful
value, which is probably not correct given that
all but one of the library calls failed.

 

[William Pursell]

In this situation, the calls are indeed more complex. Nested if
statements, while producing the correct output, would be obnoxiously
nested at 4+ levels deep. I've thought about that solution many times,
but stylistically felt like it was the wrong solution, even more so
than using the goto statement.

Then make them less complex.

 

[Ed Prochak]

In this situation, the calls are indeed more complex. Nested if
statements, while producing the correct output, would be obnoxiously
nested at 4+ levels deep. I've thought about that solution many times,
but stylistically felt like it was the wrong solution, even more so
than using the goto statement.

There is nothing wrong with using the goto statement, especially for
error recovery like this. Even if you are a dyed in the wool
structured programmer, the rule is: Branching (goto) should only go
Forward in the code, never backwards, except for well defined loops.

The misuse of goto is exactly this: haphazardly branching back to
earlier portions of code. You case does not fall into that category.
So use it if you prefer as you suggest: to branch over code which will
fail due to the earlier failure. Naming the label properly can make
things very easy to read. And the style is a good one.

HTH,
Ed

 

[Flash Gordon]

Ed Prochak wrote, On 20/03/08 17:17:

There is nothing wrong with using the goto statement, especially for
error recovery like this. Even if you are a dyed in the wool
structured programmer, the rule is: Branching (goto) should only go
Forward in the code, never backwards, except for well defined loops.

Wait until you have worked on code that branches forward in to a loop,
forward within the loop at various points, then forward out of the loop.
Then tell me that your rule is sufficient. BTW, I'm referring to real
code I've had to maintain, and it is horrible.

The misuse of goto is exactly this: haphazardly branching back to
earlier portions of code. You case does not fall into that category.
So use it if you prefer as you suggest: to branch over code which will
fail due to the earlier failure. Naming the label properly can make
things very easy to read. And the style is a good one.

I agree that it can be used usefully, but there are more options for
abusing it that you have listed.

 

[user923005]

On Mar 20, 9:15 am, myheartinamerica <[email protected]>
wrote:
[snip]

Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

It's been debated here a time or two (e.g.):
http://groups.google.com/group/comp...goto+knuth+group:comp.lang.c#09a791583ae54898
See also:
http://en.wikipedia.org/wiki/GOTO

 

[Jack Klein]

Hello,

I was doing some simple database programming and encapsulating
different
queries in functions. After each library call, I check the return
value. After
some return values, it makes no sense to continue, so I returned an
error code.
Pseudocode is something like this:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) { return -1}
retval = library_call_2()
if (retval == ERROR) { return -1}
retval = library_call_3()
if (retval == ERROR) { return -1}
/* process data */
/* cleanup handles, allocations, etc... */
return some_inportant_value

When I came in this morning, a coworker had rewritten my code. I'm
quick second
guess myself, so I assumed that his was more correct. It was something
like:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
retval = library_call_2()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
retval = library_call_3()
if (retval == ERROR) { some_important_value = MY_ERROR_CODE }
/* process data */
/* cleanup handles, allocations, etc... */
return some_inportant_value

So, he would continue to make the other library calls, even if the
previous
ones had failed (meaning that the later ones couldn't be successful).

It got me thinking, that maybe there was a better way to do this, but
I'm not
sure what that is. I'm thinking about using goto's and a label, like
this:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_2()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_3()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
/* process data */
clean_up:
/* cleanup handles, allocations, etc... */
return some_important_value


Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

Actually, since you mention in another post that the function calls
are not all clean, I prefer something like this:

result_t my_func(some_params)
{
result_t result = OK;
/* example resource allocation */
FILE *f = NULL;
data_type *ptr = NULL;

for (,,)
{
f = fopen(/*...*/);
if (!f)
{
result = FILE_ERROR;
break;
}
data_type = malloc(/*...*/);
if (!data_type)
{
result = MEM_ERROR;
break;
}
if (is_invalid(param1)
{
result = BAD_DATA_ERROR;
break;
}
/* etc. */
/* finally */
break;
}
if (f) fclose(f);
free (ptr);
return result;
}

Enclosing the test in a loop allows using break to replace goto, which
is all that C's various control structures really do, anyway.

Two advantages, however, to anyone reading the code, including
yourself a year from now:

1. All break statements in the loop go to the same place, you don't
have to search back and forth through the code to see whether all goto
targets are the same or not.

2. You know a break statement always goes forward, never backward.

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://c-faq.com/
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++
http://www.club.cc.cmu.edu/~ajo/docs/FAQ-acllc.html

 

[Ed Prochak]

Hello,

I was doing some simple database programming and encapsulating
different
queries in functions. After each library call, I check the return
value. After
some return values, it makes no sense to continue, so I returned an
error code.
Pseudocode is something like this:

[]

It got me thinking, that maybe there was a better way to do this, but
I'm not
sure what that is. I'm thinking about using goto's and a label, like
this:

[]

Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

Actually, since you mention in another post that the function calls
are not all clean, I prefer something like this:

result_t my_func(some_params)
{
result_t result = OK;
/* example resource allocation */
FILE *f = NULL;
data_type *ptr = NULL;

for (,,)
{
f = fopen(/*...*/);
if (!f)
{
result = FILE_ERROR;
break;
}
data_type = malloc(/*...*/);
if (!data_type)
{
result = MEM_ERROR;
break;
}
if (is_invalid(param1)
{
result = BAD_DATA_ERROR;
break;
}
/* etc. */
/* finally */
break;
}
if (f) fclose(f);
free (ptr);
return result;

}

Enclosing the test in a loop allows using break to replace goto, which
is all that C's various control structures really do, anyway.

Two advantages, however, to anyone reading the code, including
yourself a year from now:

1. All break statements in the loop go to the same place, you don't
have to search back and forth through the code to see whether all goto
targets are the same or not.

2. You know a break statement always goes forward, never backward.

While it is a stylistic difference, if I used the loop style I would
make sure the loop executed exactly one time, just to be safe. Bu I
still prefer the goto.
Ed

 

[Ed Prochak]

Ed Prochak wrote, On 20/03/08 17:17:



Wait until you have worked on code that branches forward in to a loop,
forward within the loop at various points, then forward out of the loop.
Then tell me that your rule is sufficient. BTW, I'm referring to real
code I've had to maintain, and it is horrible.


I agree that it can be used usefully, but there are more options for
abusing it that you have listed.

True, I wasn't trying to be exhaustive in listing abuses. And I have
seen and maintained code that abused the avoidance of goto (the worst
case was a program with a seven page while loop.)

I guess I simplified the rules a little too much. Thanks for the
follow up.
Ed

 

[Andrey Tarasevich]

...
Actually, since you mention in another post that the function calls
are not all clean, I prefer something like this:

result_t my_func(some_params)
{
result_t result = OK;
/* example resource allocation */
FILE *f = NULL;
data_type *ptr = NULL;

for (,,)
{
f = fopen(/*...*/);
if (!f)
{
result = FILE_ERROR;
break;
}
data_type = malloc(/*...*/);
if (!data_type)
{
result = MEM_ERROR;
break;
}
if (is_invalid(param1)
{
result = BAD_DATA_ERROR;
break;
}
/* etc. */
/* finally */
break;
}
if (f) fclose(f);
free (ptr);
return result;
}

Enclosing the test in a loop allows using break to replace goto, which
is all that C's various control structures really do, anyway.

Two advantages, however, to anyone reading the code, including
yourself a year from now:

1. All break statements in the loop go to the same place, you don't
have to search back and forth through the code to see whether all goto
targets are the same or not.

2. You know a break statement always goes forward, never backward.
...

I'd find such a construct more or less appropriate in a situation when
an error condition triggers a retry attempt. But as a replacement for a
'goto', i.e. when a cycle is not really a cycle, it is completely
unacceptable in my opinion. The damage to the readability of the code
done by the use of misleading cycle construct far outweighs anything a
'goto' would do.

 

[James Harris]

....

It got me thinking, that maybe there was a better way to do this, but
I'm not
sure what that is. I'm thinking about using goto's and a label, like
this:

int query_customer(const char* customer)
int retval = library_call_1()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_2()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
retval = library_call_3()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
goto clean_up;
}
/* process data */
clean_up:
/* cleanup handles, allocations, etc... */
return some_important_value

Is there some better way to do this? Is the use of goto a good example
of it's
usefulness?

So you are looking for a way to run a number of initial checks and
bring them and the main code all back together at the end to either
clean up or to achieve single exit? I wonder what you think of this
(and, indeed, if it would work; if there is a fault in my C perhaps
someone will correct the code). Its intention is to use a do ... while
zero construct which executes once providing a common exit point
without use of goto and a label.

int query_customer(const char* customer) {
do { /* execute once */
int retval = library_call_1()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
break;
}
retval = library_call_2()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
break;
}
retval = library_call_3()
if (retval == ERROR) {
some_important_value = MY_ERROR_CODE;
break;
}
/* process data */
} while 0;
/* cleanup handles, allocations, etc... */
return some_important_value;
}

 

[James Harris]

....

While it is a stylistic difference, if I used the loop style I would
make sure the loop executed exactly one time, just to be safe. Bu I
still prefer the goto.

Instead of goto and a label how do you feel about

do {
/* tests including breaks */
/* processing */
} while 0;

 

[user923005]

...


Instead of goto and a label how do you feel about

  do {
    /* tests including breaks */
    /* processing */
  } while 0;

Usually works well. The goto is still helpful in the nested case:

do {
/* tests including breaks */
/* processing */
do {
/* tests including breaks */
/* processing */
do {
/* tests including breaks */
/* processing */
do {/* what if I want to break *clear* out? */
if (obscure_condition_occurs()) break;
} while 0;
} while 0;
} while 0;
} while 0;

 

[christian.bau]

Wait until you have worked on code that branches forward in to a loop,
forward within the loop at various points, then forward out of the loop.
Then tell me that your rule is sufficient. BTW, I'm referring to real
code I've had to maintain, and it is horrible.

Some time thirty years ago it was proved that any program that uses
goto's can be rewritten without gotos. The proof is quite simple:
First, you replace all structured statements with simple statements
and gotos, so you end up with only statements of the form "expression-
statement", "goto", "if (expression) goto", with a return statement at
the end. Then you give every statement a label. Create a variable
named "label" and initialise it to the label of the first statement.
Then the rest is translated to

"while (label != <label of the return statement) {
if (label == <label of statement 1>} { statement1; label =
<label of statement2>; } // example of expression statement
else if (label == <label of statement2>} { label = <label of
goto destination>; } // example of goto statement
else if (label == <label of statement3>) { label = (expr) ?
<label of goto destination> : <label of statement4>; } // Example of
conditional goto
;;;
}

No goto statements. No structure either.

 

[Harald van Dijk]

Some time thirty years ago it was proved that any program that uses
goto's can be rewritten without gotos. The proof is quite simple: First,
you replace all structured statements with simple statements and gotos,
so you end up with only statements of the form "expression- statement",
"goto", "if (expression) goto", with a return statement at the end. Then
you give every statement a label. Create a variable named "label" and
initialise it to the label of the first statement. Then the rest is
translated to

"while (label != <label of the return statement) {
if (label == <label of statement 1>} { statement1; label =
<label of statement2>; } // example of expression statement
else if (label == <label of statement2>} { label = <label of
goto destination>; } // example of goto statement
else if (label == <label of statement3>) { label = (expr) ?
<label of goto destination> : <label of statement4>; } // Example of
conditional goto
;;;
}

No goto statements. No structure either.

enum { STMT_1, STMT_2, STMT_3, STMT_4, RETURN } label = 0;
while (label != RETURN)
switch (label) {
case STMT_1: /* statement1; */
statement1;
label = STMT_2;
continue;
case STMT_2: /* goto STMT_3; */
label = STMT_3;
continue;
case STMT_3: /* if (expr) goto STMT_1; */
if (expr) label = STMT_1;
else label = STMT_4;
continue;
case STMT_4: /* ... */
/* ... */
}

Believe it or not, but this is extremely close to being actually useful.
Make label a static variable, change continue to return, and you are
essentially looking at
http://www.chiark.greenend.org.uk/~sgtatham/coroutines.html

 

[Bartc]

Some time thirty years ago it was proved that any program that uses
goto's can be rewritten without gotos. The proof is quite simple:
First, you replace all structured statements with simple statements
and gotos, so you end up with only statements of the form "expression-
statement", "goto", "if (expression) goto", with a return statement at
the end. Then you give every statement a label. Create a variable
named "label" and initialise it to the label of the first statement.
Then the rest is translated to

"while (label != <label of the return statement) {
if (label == <label of statement 1>} { statement1; label =
<label of statement2>; } // example of expression statement
else if (label == <label of statement2>} { label = <label of
goto destination>; } // example of goto statement
else if (label == <label of statement3>) { label = (expr) ?
<label of goto destination> : <label of statement4>; } // Example of
conditional goto
;;;
}

No goto statements. No structure either.

If I understood this correctly: I take my beautifully structured program,
and decompose it into simple statements and gotos.

Then I build a framework (of if or switch statements) to eliminate the
gotos?

A lot of work to eliminate the occasional goto as in the following
(uncompiled) code:

for (i=1; i<=N; +=i)
{
if (x==2) break;

switch (x)
{
case 2:
goto mylabel;
};

};
mylabel:

I want to break out of the loop when x=2, which I can do with an 'if'
statement but not with a 'switch' statement, because the latter uses the
same 'break' keyword as used for loops. I can do this with extra code but
this would generate more clutter than a goto would.

And in fact there are many other well-structured uses of goto to eliminate a
few other deficiences in the C syntax, like breaking out of a nested loop;
this is a similar problem to the above, I can't break out of a loop using
break, from inside an nested loop.

I think 'goto' used according to certain rules and situations can be
perfectly acceptable.

The above goto elimination scheme sounds useful however when port a language
with gotos, or with different control structures, to one without.