Continuing an outer loop from an inner loop

[Ersek, Laszlo]

The single best example, common cleanup code at the end of a function,
was already given in this thread. If during all your years of
programming, you just had Dijkstra's famous letter in your mind, it may
of course never have occured to you that a goto in such kind of code
would /improve/ the structure.

There's probably not much merit in adding my own preferences to this
thread, but I tend to use the "huge tree of nested if's" Tom St Denis
seems to hate so much (IIRC -- sorry if I'm wrong, Tom!) If the tree grows
too deep, I break it up into functions. I use goto only under exceptional
conditions, where I perceive it to provide a big jump in expressive power.
Example:


----v----
From: "Ersek, Laszlo" <[email protected]>
Newsgroups: comp.unix.programmer
Subject: Re: verifying file ownership with dlopen()
Date: Wed, 5 May 2010 19:01:46 +0200
Message-ID: <[email protected]>

David Schwartz wrote in message
<f37d8057-9e6b-4e9a-a6fd-4371dadd683e@k25g2000prh.googlegroups.com>:

He needs to check every path element that leads to the library, because
either one could be a symlink.

And the check must be aware of the local system's rules for permissions.
For example, just checking the file mode may not be enough if there are
ACLs.

Once the full pathname has been tokenized, the way to do that might be
something like this. (Note - I didn't even try to compile this. It's here
only to invite criticism and/or better ideas.)

int
safeopen(const char * const * elem, size_t nelem)
{
if (0u < nelem) {
int fd;
size_t cur;

fd = open("/", O_SEARCH | O_DIRECTORY | O_NOFOLLOW);
cur = 0u;

loop:
if (-1 != fd) {
struct stat s;

if (0 == fstat(fd, &s)) {
if (cur == nelem) {
if (S_ISREG(s.st_mode) && /* other checks on the executable */) {
return fd;
}
}
else {
if (/* checks on the directory */) {
int tmp;

tmp = fd;
fd = openat(fd, elem[cur], O_NOFOLLOW
| (cur < nelem - 1u ? O_SEARCH | O_DIRECTORY : O_EXEC));

if (0 == close(tmp)) {
++cur;
goto loop;
}
}
}
}

if (-1 != fd) {
(void)close(fd);
}
}
}
return -1;
}

The returned int (if not -1) could be used with fexcve(). Unfortunately
(or not), there is no "dlfdopen()".

The code could hang indefinitely if the last component identified a FIFO.
I'd OR O_NONBLOCK with O_EXEC, if not for O_NONBLOCK being unspecified for
regular files. (I can't see anything in the openat() spec that would
require O_EXEC to fail on a FIFO (and immediately).)

[...]
----^----


Furthermore, nested if's (just like judiciously placed labels) are very
suitable for "cascading release of resources". IIRC, you said something to
the effect of "code should be condensed on the left". I sort of disagree.
Staying with the case where the if-tree still fits reasonably in a
function, I like an indentation that reflects the dependency depth of the
dependent code. (I use a "two spaces" indentation style, obviously.) With
goto's, the dependency depth is the same, but it doesn't show.

A random link:

http://en.wikipedia.org/wiki/Cyclomatic_complexity

goto's probably don't change the control flow graph, just tend to distance
the source's appearance from the CFG.

Nested if's also allow block folding in many editors and "find matching
brace".

lacos

 

[Felix Palmen]

There's probably not much merit in adding my own preferences to this
thread, but I tend to use the "huge tree of nested if's" Tom St Denis
seems to hate so much (IIRC -- sorry if I'm wrong, Tom!) If the tree grows
too deep, I break it up into functions.

There are a lot of typical tasks in a C program that need to invoke some
functions, checking errors on each of them. Take for example some code
using BSD sockets. Splitting these tasks doesn't make sense.

Now, if you write it like this:

if (do_foo() >= 0)
{
if (do_bar() >= 0)
{
if (do_baz() >= 0)
{
...

It'll get unreadable very soon. The much better style, even representing
the sequential nature of the code optically, is this:

if (do_foo() < 0)
{
return -1;
}

if (do_bar() < 0)
{
return -1;
}

....

do_real_work()

....

And, in case cleanup code is needed, replace the return statements with
gotos.

goto's probably don't change the control flow graph, just tend to distance
the source's appearance from the CFG.

I'd argue that you ultimately want readable (as in: human-readable)
code. So, I consider it much more important to reflect the semantics of
the code in it's structure, rather than the control flow graph.

In most XML-based languages (consider e.g. BPEL, XLANG, ...), you are
very much bound to the control flow. This results in the typical
application of such languages, to describe highly sequential processes.
Anything else would result in ridiculously deep nesting.

Regards,
Felix

 

[BartC]

I was wondering what style of programming is preferred when continuing
an outer loop from within an inner loop. As far as I see it, there

if(foundIt)
{
continue;
}

You seem to like braces and white space (except where it would be useful).
What's wrong with:

if (foundIt) continue; /* ? */

if(<condition3>)
{
goto continueouter;
}
}
goto skipcontinue;
continueouter:
continue;
skipcontinue:
...

As written, this second form is more confusing; most of the these lines seem
to contain 'continue' in some form. This would be better as:

if (foundIt) goto nextouter;
.....
nextouter:
}

as I think you've posted elsewhere.

Either form is OK I think. Certainly for getting code to work, use whatever
you like. For code to be read by someone else (or perhaps ported to a
language without gotos), I'd think twice about using goto.

(This problem would be very easily solved by a bit of extra syntax (continue
n or continue outer for example). But no-one seems to like adding extra
syntax to a language even though it is fairly trivial to implement (eg. ten
lines of code in one of my projects). It's far better of course for hundreds
of thousands of programmers to each add hundreds of lines of convoluted code
instead.)

 

[BartC]

........

This got posted unintentionally. Please ignore.

 

[Malcolm McLean]

There are a lot of typical tasks in a C program that need to invoke some
functions, checking errors on each of them. Take for example some code
using BSD sockets. Splitting these tasks doesn't make sense.

Now, if you write it like this:

if (do_foo() >= 0)
{
  if (do_bar() >= 0)
  {
    if (do_baz() >= 0)
    {
      ...

It'll get unreadable very soon. The much better style, even representing
the sequential nature of the code optically, is this:

if (do_foo() < 0)
{
  return -1;

}

if (do_bar() < 0)
{
  return -1;

}

...

do_real_work()

Really what you are saying here is that error conditions obscure the
normal flow of the code.

try .. throw ... catch

is the C++ and Java answer, which brings its own problems.

 

[Felix Palmen]

Really what you are saying here is that error conditions obscure the
normal flow of the code.

At least, that's a common aspect.

try .. throw ... catch

is the C++ and Java answer, which brings its own problems.

You could also argue that a throw - catch inside one method is (in terms
of control flow) equivalent to a goto <error-label>, while a catch
somewhere up the caller-chain would be a global goto, a longjmp in C.

Regards,
Felix

 

[Malcolm McLean]

At least, that's a common aspect.



You could also argue that a throw - catch inside one method is (in terms
of control flow) equivalent to a goto <error-label>, while a catch
somewhere up the caller-chain would be a global goto, a longjmp in C.

The difference is that you can only jump forwards (and up). Backwards
gotos are far more problematic in terms of creating spaghetti code.

I use the if(err) goto error_exit; construct a lot, however.

 

[Wolfgang.Draxinger]

Am Tue, 14 Sep 2010 10:35:29 -0700 (PDT)

I was wondering what style of programming is preferred when continuing
an outer loop from within an inner loop. As far as I see it, there
are two possibilities, which I have given in pseudo-code below. The
first follows the strict, structural programming rule of only using
breaks and continues to control flow, while the second uses gotos.
Personally I prefer the second since it seems clearer, seems like it
would be easier to optimize, and doesn't use an artificial control
variable. What do you think?

I too prefer gotos in such a case. If you're disciplined and know what
you're doing there's nothing wrong with it.

You post brought me to an idea, something that may be added to the
next C version: Labeled loops. I'm thinking of something like this
(since this would be an extension to C99, I'm using C99 features down
there (variable declaration in for clause, mixed code and declaration).
Also you'd like to know if you broke from a inner loop. For that I
suggest a virtual label break, that's used like switch, where the case
refers to the label of the inner loop, default case if the break
happend on outermost loop level. As a shorthand just break: without
cases if you're only interested on an outmost loop break. Following
such a break:-scope with else, the else-scope is executed if no break
happened.

/* ... */

int j_from_i(int i);
#define MAX_J 1000
extern int N;

void foo(void)
{
for : outer (int i=0; i<N; i++) {
int j = j_from_i(i);

while : inner (j) {
if(j > MAX_J)
break outer;

for(int k=j; k>0; k--) {
/* ... */
if(k == i)
break inner;
}
}
} break: {
case inner:
break;
default:
}
}

/* ... */

Another thing that would be valueable would deinitializers, e.g.
putting the commands perform (in opposite order) whenever a scope is
left, after the given deinitialzer has been reached. To avoid adding a
new keyword I suggest using return as a (virtual) scope-label, i.e. a
scope following immetiately after return would be delayed until the
scope is left. Since return is a keyword, using it like a label would be
unambigous.

return-scopes are executed in opposite order of definition. Also
within a return-scope using control flow statements that'd leave the
return-scope would be illegal.

#include <stdio.h>

struct spam;
struct spam * new_spam(void);
void free_spam(struct spam * s);

struct eggs;
struct eggs * new_eggs(void);
void free_eggs(struct eggs * e);

struct spam_n_eggs;
struct spam_n_eggs * new_spam_n_eggs(struct spam * s, struct eggs * e);
void free_spam_n_eggs(struct spam_n_eggs * se);
void spam_n_eggs_process(struct spam_n_eggs * se, int i);

extern int debug;

void bar(void)
{
struct spam * s = newspam();
return: {
free_spam(s);

if(debug)
fprintf(stderr, "DEBUG: spam at %p freed.\n",
s);
}
if(s) {
if(debug)
fprintf(stderr, "DEBUG: new spam at %p.\n", s);
} else {
if(debug)
fprintf(stderr, "DEBUG: allocating spam
failed.\n");
return;
}

struct eggs * e =
return: {
free_eggs(e);

if(debug)
fprintf(stderr, "DEBUG: eggs at %p freed.\n",e);
}
if(e) {
if(debug)
fprintf(stderr, "DEBUG: new eggs at %p.\n", e);
} else {
if(debug)
fprintf(stderr, "DEBUG: allocating eggs
failed.\n");
return;
}

if(debug)
fprintf(stderr, "DEBUG: processing spam and
eggs.\nDEBUG: ");
for(i=0; i<10; i++) {
struct spam_n_eggs * se = new_spam_n_eggs(s, e);
return: free_spam_n_eggs(se);
if(!se)
break;

spam_n_eggs_process(se);

if(debug) {
fprintf(stderr, "%d... ", %i);
fflush(stderr);
}
} break: {
if(debug)
fprintf(stderr, "error.\n");
} else if(debug)
fprintf(stderr, "done.\n");
}

Running this program in debug mode would print:

DEBUG: new spam at 0xabcd0123.
DEBUG: new eggs at 0xcdef2345.
DEBUG: processing spam and eggs.
DEBUG: 0... 1... 2... 3... 4... 5... 6... 7... 8... 9... done.
DEBUG: eggs at 0xcdef2345 freed.
DEBUG: spam at 0xabcd0123 freed.

If allocating a spam_n_eggs fails:

DEBUG: new spam at 0xabcd0123.
DEBUG: new eggs at 0xcdef2345.
DEBUG: processing spam and eggs.
DEBUG: 0... 1... 2... 3... error.
DEBUG: eggs at 0xcdef2345 freed.
DEBUG: spam at 0xabcd0123 freed.

If allocating the eggs fails:

DEBUG: new spam at 0xabcd0123.
DEBUG: allocating eggs failed.
DEBUG: spam at 0xabcd0123 freed.


Having such a scope finalizing facility would eleminiate 99% of the
cases were today goto is used in a usefull way.

Just a few ideas. Comments?


Wolfgang

 

[lawrence.jones]

There's never a good use for goto!

Absolute statements are always wrong.

 

[Ian Collins]

At least, that's a common aspect.

Some languages with exceptions also provide most if not all of the
solutions to those problems. If you have exceptions, you have to be
able to write exception safe code. To write exception safe code, you
have to be able to automate clean up, so you can use early return safely.

 

[Malcolm McLean]

Some languages with exceptions also provide most if not all of the
solutions to those problems.  If you have exceptions, you have to be
able to write exception safe code.  To write exception safe code, you
have to be able to automate clean up, so you can use early return safely.

I've never seen a language with good error-handling facilities. That
doesn't mean there aren't such languages.
Of course there are plenty of situations - eg video games - where you
can't have errors. Our games only ever failed for two reasons, the
drive door was open or the disk was dirty. On error, the user had to
remove the disk, wipe it, and restart the game from the last save
point. That always fixed the problem.

 

[Ian Collins]

I've never seen a language with good error-handling facilities. That
doesn't mean there aren't such languages.

That wasn't the point I was making. I was replying to your post (sorry
I replied to the wrong message):

"try .. throw ... catch

is the C++ and Java answer, which brings its own problems."

Languages with exceptions also provide a mechanism to clean up when an
exception is thrown, either through automatic object destruction (C++)
or finalisers (Java, PHP and others).

 

[Felix Palmen]

Or as I prefer to write it:

if (op() == FAIL)
{
/* first op failed */
}
else if (op() == FAIL)
{
/* second op failed */
}
else if (op() == FAIL)
{
/* third op failed */
}
else
{
/* success */
}

That's one possible style I'd recommend (and probably Tom would, too).

But ...

I'm very predjudiced against goto, and dislike excessive nesting.

Where exactly do you put the cleanup code in this case? Without using
goto, you'll either start setting some funny flags or you'll repeat the
code.

Regards,
Felix

 

[Malcolm McLean]

Or as I prefer to write it:

if (op() == FAIL)
{
    /* first op failed */}

else if (op() == FAIL)
{
    /* second op failed */}

else if (op() == FAIL)
{
    /* third op failed */}

else
{
    /* success */

}

One problem with that is that

if(getcursorpos(&pt) == -1)
{
exit(EXIT_FAILURE);
}
else if( ... )

makes it look like the main purpose of getcursorpos() is to return a
flag on which to make the decison whether or not to abort the program.

err = getcursorpos(&pt);
if(err == -1)
goto error_exit;

makes the situation a little clearer, though there's still more error
code than normal execution code.

 

[Ersek, Laszlo]

How about:

if (op() != OK) {}
else if (op() != OK) {}
else if (op() != OK) {}
...

Or as I prefer to write it:

if (op() == FAIL)
{
/* first op failed */
}
else if (op() == FAIL)
{
/* second op failed */
}
else if (op() == FAIL)
{
/* third op failed */
}
else
{
/* success */
}

I'm very predjudiced against goto, and dislike excessive nesting.

The above code isn't complete enough. If each step augments a "transaction
in flight", and any step can fail before the final commit, one needs
"cascading rollback".

The "excessive nesting" way:

enum result
xact(void)
{
if (SUCCESS == op1()) {
if (SUCCESS == op2()) {
if (SUCCESS == op3()) {
commit(); /* can't fail */
return SUCCESS;
}
rollback2();
}
rollback1();
}

return FAIL;
}

Note that (a) this makes it easy to constrain declarations to the
narrowest applicable scopes, even under C89, and (b) rollbacks happen
naturally in reverse order. (The first thing one types into the
just-opened scope dependent on the SUCCESS of op_k() is rollback_k(), then
moves one line up and proceeds with calling and checking op_(k+1)(), or
with declarations necessary for that.)


The "goto way":

enum result
xact(void)
{
if (FAIL == op1()) {
goto bail0;
}

if (FAIL == op2()) {
goto bail1;
}

if (FAIL == op3()) {
goto bail2;
}

commit();
return SUCCESS;

bail2:
rollback2();

bail1:
rollback1();

bail0:
return FAIL;
}

Declarations can be constrained in C99 style (but that delays only the
beginnings, and doesn't bring the ends as near the top as desirable), or
by introducing "gratuitous" braces (which leads to nesting, which this
style tries to avoid). Reverse ordering of rollback steps needs extra
attention. (I think -- I never use this style, so one might mechanize it
easily as well.)

On the other hand, the quoted code can be completed like this (doing away
with "else"'s) -- among other ways:

enum result
xact(void)
{
if (FAIL == op1()) {
return FAIL;
}

if (FAIL == op2()) {
rollback1();
return FAIL;
}

if (FAIL == op3()) {
rollback2();
rollback1();
return FAIL;
}

commit();
return SUCCESS;
}

For n transactional steps, this needs O(n^2) rollback steps to be typed
out, and correct reverse ordering can be screwed up in about n-2 blocks.
'Nuff said.

Or else, the quoted code can be completed (among other ways) in this
different manner:

enum result
xact(void)
{
enum result res1,
res2 = FAIL,
res3 = FAIL;

res1 = op1();

if (SUCCESS == res1) {
res2 = op2();
}

if (SUCCESS == res2) {
res3 = op3();
}

if (SUCCESS == res3) {
commit();
return SUCCESS;
}

if (SUCCESS == res2) {
rollback2();
}

if (SUCCESS == res1) {
rollback1();
}

return FAIL;
}

A lot of unnecessary "if"'s can be executed. Explicit temporary variables
represent stretches of code that in fact correspond to nested scopes.
'Nuff said.

The only real contenders (from those mentioned) are the "deep nesting way"
and the "goto way". Due to its naturally narrow scoping of declarations
(even under C89 *and* wrt. scopes' ends), and due to its easy reverse
ordering of rollback steps, the nesting way wins for me.

lacos

 

[Shao Miller]

... ... ...
The "goto way":

enum result
xact(void)
{
if (FAIL == op1()) {
goto bail0;
}

if (FAIL == op2()) {
goto bail1;
}

if (FAIL == op3()) {
goto bail2;
}

commit();
return SUCCESS;

bail2:
rollback2();

bail1:
rollback1();

bail0:
return FAIL;
}

Declarations can be constrained in C99 style (but that delays only the
beginnings, and doesn't bring the ends as near the top as desirable), or
by introducing "gratuitous" braces (which leads to nesting, which this
style tries to avoid). Reverse ordering of rollback steps needs extra
attention. (I think -- I never use this style, so one might mechanize it
easily as well.)

... ... ...

You can "forget" about keeping track of an 'op2()' failure requiring a
'bail1' jump and a 'rollback1()' roll-back by grouping
operations/resources with their labels in the manner offered below. If
the time comes (such as in 'USE_BAZ') that a new resource is part of the
transaction, there aren't any indentation changes and previous code
remains untouched. That is, brand new code can be typed and the 'diff'
should not note any previous lines as being touched; pure additions.

Of course, that fact has different value to different people.

#include <stdlib.h>
#include <stdio.h>

#define USE_BAZ 0

struct foo {
int x;
};

struct bar {
long x;
};

struct baz {
double x;
};

static const char *const test(void) {
const char *ret = "Failure.";
struct foo *resource1;
struct bar *resource2;
#if USE_BAZ
struct baz *resource3;
#endif

/**
* Resource allocation section.
**/
resource1 = malloc(sizeof resource1);
if (!resource1) {
fprintf(stderr, "'malloc' failed for 'resource1'.\n");
goto err_resource1;
}

resource2 = malloc(sizeof resource2);
if (!resource2) {
fprintf(stderr, "'malloc' failed for 'resource2'.\n");
goto err_resource2;
}

#if USE_BAZ
resource3 = malloc(sizeof resource3);
if (!resource3) {
fprintf(stderr, "'malloc' failed for 'resource3'.\n");
goto err_resource3;
}
#endif

/**
* Processing section.
**/
/* ... */
ret = "Success.";

/**
* Cleanup section.
**/
#if USE_BAZ
free(resource3);
err_resource3:
#endif

free(resource2);
err_resource2:

free(resource1);
err_resource1:

return ret;
}

int main(void) {
puts(test());
return 0;
}

 

[Ersek, Laszlo]

Elaborating further:

The "excessive nesting" way:

enum result
xact(void)
{
if (SUCCESS == op1()) {
if (SUCCESS == op2()) {
if (SUCCESS == op3()) {
commit(); /* can't fail */
return SUCCESS;
}
rollback2();
}
rollback1();
}

return FAIL;
}

The "goto way":

enum result
xact(void)
{
if (FAIL == op1()) {
goto bail0;
}

if (FAIL == op2()) {
goto bail1;
}

if (FAIL == op3()) {
goto bail2;
}

commit();
return SUCCESS;

bail2:
rollback2();

bail1:
rollback1();

bail0:
return FAIL;
}

In reality I usually like to log which step failed. I must honestly admit
the "goto way" reacts better to that modification -- its structure doesn't
change at all, while in my preferred way, the SUCCESS checks flip to FAIL
checks (less importantly), and for each "if", an "else" branch is added
(more importantly). With C99 style block scope declarations (and not
placing weight on how early visibilities end) I can see why one would like
the "goto way" more.


enum result
xact(void)
{
if (FAIL == op1()) {
log1();
}
else {
if (FAIL == op2()) {
log2();
}
else {
if (FAIL == op3()) {
log3();
}
else {
commit(); /* can't fail */
return SUCCESS;
}
rollback2();
}
rollback1();
}

return FAIL;
}


enum result
xact(void)
{
if (FAIL == op1()) {
log1();
goto bail0;
}

if (FAIL == op2()) {
log2();
goto bail1;
}

if (FAIL == op3()) {
log3();
goto bail2;
}

commit();
return SUCCESS;

bail2:
rollback2();

bail1:
rollback1();

bail0:
return FAIL;
}


lacos

 

[August Karlstrom]

Total nonsense. Convoluted loop conditions blur the meaning. break and
goto significantly simplify such constructs.

It's not "total nonsense". It is a matter of preference or schools. I'm
with Dijkstra and you seem to be with Knuth in this matter.


/August

 

[Felix Palmen]

It's not "total nonsense". It is a matter of preference or schools. I'm
with Dijkstra and you seem to be with Knuth in this matter.

Dijkstra certainly didn't have those examples of good and well
thought-of goto usage in mind. When you want to point out a big mess,
you just HAVE to be radical in your first attempt.

Regards,
Felix

 

[luserXtrog]

void function2(void)
  while(<condition1>)
  {
    while(<condition2>)
    {
      if(<condition3>)
      {
        goto continueouter;
      }
    }
goto skipcontinue;
continueouter:
    continue;
skipcontinue:
...
  }

}

Even if you must goto, The Elements of Programming Styles tells
us that you should never ever goto around another goto.
You should reverse the test and do a single jump.

But here even that doesn't appear necessary, if you put the
continue at the end.

void fun2 (void) {
while (<cond1>) {
while (<cond2>) {
if (<cond3>) goto outer;
}
/* ... */
outer: continue;
}
}