slightly OT: error handling conventions

[Sheldon Simms]

ah. It was just you said/implied in your original post something along
the lines of "some professor" said we shouldn't use goto's. I thought
Dijkstra rated more consideration than that.

I understand now. The "some professor" I was talking about wasn't
Dijkstra (at least, I don't think so). It was a guy who taught a
structured programming methodology in which the programmer should
literally draw boxes inside boxes to design a program.
Something like:

int myfunc (int a, int b)
+--------------------------------+
|{ |
| while (a < b) |
|+---------------------------+ |
|| { | |
|| ++a; | |
|| --b; | |
|| if (a == b) | |
||+---------------------+ | |
||| { | | |
||| ++a; | | |
||| } | | |
||+---------------------+ | |
|| } | |
|+---------------------------+ |
| return a; |
+--------------------------------+

 

[rihad]

For code where I have multiple resources to find, use, then release I
often use the idea of a 'runlevel'. Basically, it's this:

int do_stuff(char * iname, char * oname) {
int rl=0; /* the 'runlevel' variable */
int rv=ERROR; /* the error return value */
FILE * inf;
FILE * outf;
void * mem;

inf=fopen(iname,"r");
if (!inf) goto on_err;
else rl++; /* opened a resource, go up a level */

outf=fopen(oname,"w");
if (!outf) goto on_err;
else rl++; /* opened a resource, go up a level */

mem=malloc(WHATEVER_SIZE);
if (!mem) goto on_err;
else rl++; /* opened a resource, go up a level */

/* do stuff with inf, outf and mem */

rv=0; /* the success return value */

on_err:
switch(rl) {
case 3: free(mem);
case 2: fclose(outf);
case 1: fclose(inf);
case 0: /* nothing to clean up */
}

return rv;
}

Nice. I'm using this approach which I find a bit easier to follow and more
defensive (runtime !NULL checks) than yours.

int do_stuff(char * iname, char * oname) {
int rv=ERROR; /* the error return value */
FILE * inf = 0;
FILE * outf = 0;
void * mem = 0;

inf=fopen(iname,"r");
if (!inf) goto on_err;

outf=fopen(oname,"w");
if (!outf) goto on_err;

mem=malloc(WHATEVER_SIZE);
if (!mem) goto on_err;

/* do stuff with inf, outf and mem */

rv=0; /* the success return value */
goto exit:

on_err:
free(mem);
if (outf) fclose(outf);
if (inf) fclose(inf);
exit:
return rv;
}

 

[rihad]

The value of b in

I'd say that the calling function MUST NOT make any assumptions about
the value of b when func_that_mallocs() returns failure.

This one's easy: if the contract with func_that_mallocs() clearly states that
the value of 'b' is indeterminate on error, then the caller MUST make 'b'
cleanable again. Namely, b = NULL right before the goto.

FWIW, a non-goto
version of the OPs function would be:

int my_func() {
enum { INIT, GOT_A_MEM, DID_A, GOT_B_MEM } rslt = INIT;
char *a, *b;
int x, ret = ERROR;

a = malloc(some_number);
if (a != NULL) rslt = GOT_A_MEM;

if (rslt == GOT_MEM) {
x = another_function(a);
if (x != ERROR_CONDITION) rslt = DID_A;
}

if (rslt == DID_A) {
x = func_that_mallocs(&b);
if (x != ERROR_CONDITION) rslt = GOT_B_MEM;
}

switch(rslt) {
case GOT_B_MEM: ret = NO_ERROR; free(b);
case DID_A:
case GOT_A_MEM: free(a);
}

return ret;
}

Sorry, but this does look like over-design of a very simple concept to me. It's
quite hard to read, introduces quite a few unneded logic elements (why the heck
do we need GOT_A_MEM if `a' itself is authoritative of its own value?!).

Nothing beats gotos in this respect, they are clearly the clearest and easiest
to read solution C can offer.

 

[Ed Morton]

rihad wrote:

Nice. I'm using this approach which I find a bit easier to follow and more
defensive (runtime !NULL checks) than yours.

int do_stuff(char * iname, char * oname) {
int rv=ERROR; /* the error return value */
FILE * inf = 0;
FILE * outf = 0;
void * mem = 0;

Shouldn't you init them to NULL rather than zero?

inf=fopen(iname,"r");
if (!inf) goto on_err;

outf=fopen(oname,"w");
if (!outf) goto on_err;

mem=malloc(WHATEVER_SIZE);
if (!mem) goto on_err;

"mem" is never free-ed in the success case.

/* do stuff with inf, outf and mem */

Or is "do stuff with...mem" intended to include duplicating the
"free(mem)" from the error legs?

rv=0; /* the success return value */
goto exit:

on_err:
free(mem);
if (outf) fclose(outf);
if (inf) fclose(inf);

Kind of a shame you have to test outf and inf again since you already
tested them as one way to get here in the first place.

Ed.

 

[Ed Morton]

This one's easy: if the contract with func_that_mallocs() clearly states that
the value of 'b' is indeterminate on error, then the caller MUST make 'b'
cleanable again. Namely, b = NULL right before the goto.

That still invokes implementation-specific behavior. In this case, the
right way to code func_that_mallocs() is:

int func_that_mallocs(char **b) {
char *tmp = malloc(...);
.... /* ensure tmp is not NULL and do other things */
if (today_is_tuesday) {
free(tmp);
return ERROR_CONDITION;
} else {
*b = tmp;
return SUCCESS;
}
}

so the contract is that b will be unchanged if func_that_mallocs()
returns an error (and so will still be set to NULL if it was originally
initialised to NULL in the calling function) but you're missing the
point that we're using malloc and free as a specific case of allocating
and deallocating resources to discuss the general philosophy, not trying
to find the best way to deal specifically with NULL pointers wrt
malloc/free, though I do think the general approach works just fine in
this case too.

Sorry, but this does look like over-design of a very simple concept to me. It's
quite hard to read,

Really? I could buy you saying ocver-design for this small example, but
I'm amazed you'd find it hard to read.

introduces quite a few unneded logic elements (why the heck

do we need GOT_A_MEM if `a' itself is authoritative of its own value?!).

Because, in general, "a" isn't and we don't want to overload it. For
example if "a" is NULL does that mean that allocation failed or that
allocation was never attempted or that allocation succeeded but then a
was later released? Maybe it doesn't matter in this code, but in other
code it might.

Nothing beats gotos in this respect, they are clearly the clearest and easiest
to read solution C can offer.

Since we're having this discussion clearly neither solution is "clearly
the clearest...".

Ed.

 

[rihad]

rihad wrote:


Shouldn't you init them to NULL rather than zero?

Apart from being a zero value constant, 0 is also the null pointer constant. It
doesn't mean the integer value of 0 is converted to a pointer value or anything
like that:

void *p;
int i;
p = i = 0; /* illegal */
p = 0; /* legal */
p = NULL; /* a bit verbose but otherwise harmless */

Sorry if you already knew all this.

"mem" is never free-ed in the success case.

Oops, just a brain fart. Neither the files are closed. Well, In this example we
just get rid of goto exit. What I was thinking of was when you return an
allocated resource:

resource_t *resource = malloc(...);
if (!resource) goto on_err;

/* main code with normal return */
goto exit;

on_err:
free(resource);
resource = 0;
free(some_helper_resource);
exit:
return resource;

But our case is even simpler.

Or is "do stuff with...mem" intended to include duplicating the
"free(mem)" from the error legs?


Kind of a shame you have to test outf and inf again since you already
tested them as one way to get here in the first place.

You can get here from *anywhere* within the function body. So the actions taken
would normally be generic, i.e. unoptimized. Nothing to lose sleep over, if you
ask me.

 

[Ed Morton]

rihad wrote:

Apart from being a zero value constant, 0 is also the null pointer constant.

Not exactly, it's one possible value of the NULL pointer constant.

Ed.

 

[Christian Bau]

Christian Bau wrote:


I'd say the opposite - that introducing multiple goto destinations is
completely unnecessary and only complicates matters for no gain in
readability at all when you can use a local variable and a switch statement.

What if the function had 10 failing conditions/cleanups? Would you
really introduce 10 goto destinations?

Yes.

What if you couldn't use a simple fall-through at the end to release
resources? With my version you can test the variable as often as
necessary for whatever combination of values are appropriate, e.g. you
could do this instead of a "switch":

if (rslt == A || rslt == B) {
release(x);
}
if (rslt == B || rslt == C) {
release(y);
}

whereas with yours the equivalent would at it's most readable be
something like:

A: release(x);
goto END;
B: release(x);
release(y);
goto END;
C: release(y);
goto END;
END: return;

i.e. nested gotos and duplicated code.

Nice try. I said that your method, when it is applicable, is clumsy and
can be replaced by something simpler. You claim that when your clumsy
method is not applicable you have an alternative method. Guess what: If
the clumsy method is not applicable and its elegant replacement is not
applicable either, I am absolutely free to use any alternative method
that I like. switch didn't work, so you replaced it with a series of
if's. Why would I stay with goto if goto doesn't work?

goto can be used as a very simple pattern for very simple resource
allocations: Resources are allocated sequentially, and when allocation
fails they are deallocated sequentially.

The next more complicated pattern is having a state variable for each
resource that was allocated; each state variable is initialised at the
very beginning of a function and at cleanup time each state variable is
checked individually; that makes it possible to allocate any subset of
resources in any order. I would really avoid creating complex
dependencies like you did: Just have two boolean variables
"x_is_allocated" and "y_is_allocated", initialize them to false, then
change them to true when one of the resources is allocated.

 

[Christian Bau]

On Wed, 22 Oct 2003 07:06:27 UTC, Christian Bau

some non-transparent code.
Wrong.

I've learned in my first days of programming (the dark age where not
even assembly was available) that

- making a well design is more than the half work.
- goto is evil and should be avoided whenever possible
(o.k., in very low language as mashine code
or assembly it is truly impossible but for that
you can always design the code so that there is no
long goto.
And with C you can design the code always in a
manner that NO goto is needed. Make the design
right and you gets always transparent code.
- find a time inexpensive algoritm before you starts coding
to reduce runtime when a loop tends to be time expansive
That means: start to optimise time before the first statement
gets written.

Between your first days of programming and today, did you learn anything
else? Do the rules that you learnt as a beginner still apply to you, or
have you grown since then?

 

[Christian Bau]

It was clear. What I want to express is that there is no *single* approach.
Just because you can write everything without goto (or switch) doesn't mean
that this will always lead to the most readable code.

Some people have this blockage in their brain where they state: Goto is
evil. Code using goto is unreadable. Look at this code: It contains
goto's, therefore it is unreadable, which proves that every bit of code
containing goto is unreadable.

You can't argue with that, you can only hope that eventually they grow
up.

 

[Christian Bau]

This is a highly artificial example. There is NO point in designing
func_that_mallocs that way. If it *has* to free *b, then it can also
trivially set it to NULL after calling free on it. The value of b in
my_func() MUST NOT be indeterminate after calling func_that_mallocs.

And everyone who writes such a function _and does not document that *b
will either b a null pointer or an allocated pointer_ should be punched
in the face. As a user of such a function, you know that only an idiot
would write code that leaves *b set to garbage, but you can't rely on
it, because the function _might_ have been written by an idiot.

 

[Christian Bau]

I understand now. The "some professor" I was talking about wasn't
Dijkstra (at least, I don't think so). It was a guy who taught a
structured programming methodology in which the programmer should
literally draw boxes inside boxes to design a program.
Something like:

int myfunc (int a, int b)
+--------------------------------+
|{ |
| while (a < b) |
|+---------------------------+ |
|| { | |
|| ++a; | |
|| --b; | |
|| if (a == b) | |
||+---------------------+ | |
||| { | | |
||| ++a; | | |
||| } | | |
||+---------------------+ | |
|| } | |
|+---------------------------+ |
| return a; |
+--------------------------------+

There are two kinds of program structures: Those that I can easily
handle in my head without the use of paper, and those that wouldn't fit
on any kind of paper anyway

 

[Christian Bau]

on_err:
free(mem);
if (outf) fclose(outf);
if (inf) fclose(inf);

Kind of a shame you have to test outf and inf again since you already
tested them as one way to get here in the first place.[/QUOTE]

He is using very simple invariants for the resources he allocated: If
mem is a null pointer then it doesn't need deallocating, if it is not
null then it needs deallocating. If outf is not null then it needs to be
closed, otherwise it doesn't. Same for inf.

A very useful programming strategy: Decide on your invariants, and make
sure that they hold from the start to the end of the function. A very
robust strategy that will work even if the code in between changes
significantly.

 

[Richard Heathfield]

IMO this method has two drawbacks. It leads to functions doing
little with *many* lines.

Too many lines? That's a drawback, huh? Presumably you have to pay extra for
newlines. I must admit I had no idea it was a problem. Okay, watch this:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "commsi.h"
int TCPGetMessage(TCP*connection,char**Message,size_t*MaxSize,const
char*terminator){int rc=TCP_SUCCESS;size_t result=0;int complete=0;
size_t bytes=0;char data[256]={0};size_t term_len=strlen(terminator);
while(TCP_SUCCESS==rc&&!complete){memset(data,0,sizeof data);rc=
TCPReceive(connection,data,sizeof data);if(TCP_SUCCESS == rc){rc=
TCPGetLastRead(connection,&result);if((bytes+result+1)>*MaxSize){
size_t newsize=(3* *MaxSize)/2;char *tmp=NULL;if(newsize<(size_t)
(bytes +result+1)){newsize=bytes+result+1;}tmp=realloc(*Message,
newsize);if(tmp==NULL){rc=TCP_GETMESSAGE_LOWMEM;}else{*Message=tmp;
*MaxSize=newsize;}}}if(TCP_SUCCESS==rc){char*p=*Message + bytes;
memcpy(*Message+bytes,data,result);(*Message)[bytes+result]='\0';
if(bytes>term_len-1){p-=term_len-1;}if(strstr(p,terminator)!=NULL)
{complete=1;}bytes+=result;}}return rc;}

So that's the line problem sorted.

In case of more preparation (more files,
some mallocs, sockets, whatever) it gets messy pretty quick. The
method does not scale very good.

That has not been my experience. YMMV.

Jirka

PS: Didn't you promise, you will not get involved in a style discussion
anymore? ;-)

I don't believe so. If you think differently, please feel free to provide a
reference.

 

[Richard Heathfield]

Sorry, I'm not Ian, but I would like to ask a question of my own:

What would be the advantage of eliminating goto in a situation
like this?

When reading code, I find goto irritating, because it makes me search
through the code looking for a label. Since I read my own code quite a lot,
I tend to avoid using goto myself. (To say "tend to avoid" is perhaps an
understatement, since I haven't used a goto in real C code in all the years
I've been using the language. I was a big GOTO fan in BASIC, but I
eventually discovered that my BASIC programs, whilst quite cool to my
unjaundiced eye, were largely write-only.)

I once got involved in a large, er, "discussion" on the German C
group about goto. No matter what situation was presented, the
majority of those participating agreed that the use of goto was
an abomination that should be avoided at all costs. Many of the
same people also claimed to refuse to use break and continue.

I wouldn't say "at all costs", but I would avoid goto if there were a more
readable choice. So far, there always has been, at least for me. As for
break, I use it only in switches. Whilst there is a case to be made for
continue in dismissing early exceptions to a loop - for example:

if(line[0] == '#') /* comment - skip it */
{
continue;
}

nevertheless I tend to use continue only to self-document empty loops in an
uncompromising manner (the lack of loop contents could be said to be
self-documenting for an empty loop, but it could also be read as
"programmer forgot loop contents"!).

Their reasoning for this was hard for me to understand, since I
lacked the cultural background, but apparently some influential
professor had some ideas about program design which led most of
them to require all programs to be designed by drawing boxes
inside of boxes. Control was only permitted to flow into the
top of a box or out of the bottom of a box.

That restriction makes code much easier to visualise, IMHO.

This was all nice, and I understood it to be a graphical approach
to structured programming, but what I didn't understand was the
attitude that *absolutely no* deviation from this schema was to
be tolerated.

No, I don't understand that either. The overriding priority is, of course,
correctness. After that, though, I'd go for readability. I'm ready to drop
my structural ascetism tomorrow, or even today, if I happen to find an
alternative that I find more readable. And that alternative, for all I
know, might well include goto.

 

[Richard Heathfield]

There are two kinds of program structures: Those that I can easily
handle in my head without the use of paper, and those that wouldn't fit
on any kind of paper anyway

I have some N-dimensional paper for sale. Only US$1000000 per dimension per
sheet.

 

[rihad]

rihad wrote:


Not exactly, it's one possible value of the NULL pointer constant.

Not *exactly*. ((void *) 0) is a common way to represent 0 - the null pointer
constant - behind the NULL macro. On such systems use of NULL can be convenient
- buggy assignments of NULL to non-pointer types will be flagged out early. But
what NULL expands to is entirely implementation-defined, it can be 0, too
(especially on dual C/C++ systems). That's why I chose to not rely on NULL at
all and use 0 exclusively. It's a tiny bit more extendible too, since 0 is a
good rv for just about anything (and passes as a reasonably good initializer to
even aggregate types).

 

[Irrwahn Grausewitz]

I've learned in my first days of programming (the dark age where not
even assembly was available) that [...]
- goto is evil and should be avoided whenever possible

<snip>

There must have been important changes to the universal timeline
I wasn't aware of yet.

 

[The Real OS/2 Guy]

Actually it was completely obvious what was going on there.
If you were confused by it, perhaps you should give up on
programming.

I sayed only that the code was non-transparent. Now ord that is not
was understandable.

Oh really? You've been programming since before assembly
was available? When was that then, 1948?

1970. Yes, there was no assember available. There was not even a
simple debugger available. There was no OS for that mashine available.
The mashine was unable to understund hex code!

CPU : ADS2100, 12 decimal digit word
memory : 2 K word core memory; optionally 3 or 4 K word.
keyboard: 102 keys german
printer : 1 pinwheel, for 2 continous paper pathways + magnetic ledger
magnetic ledger card (80 columns)
periphery: none

Storage space: 5 x 6 qm.

Were they teaching that in 1948? I didn't know that.

In any case, you are wrong. goto is not evil.
You can do evil things with goto, but that's not
the same as goto being evil.

Goto IS evil as it destroys the control flow. There is not a single
cause where goto may useful. Make a good design of your program, your
functions and you will not even come of the idea that you may need a
goto.

I could drive at high speeds through the middle of the
pedestrian zone in downtown Cologne at noon on Saturday,
but that doesn't make cars evil.


True, but there are also times in C programming when goto is
an appropriate construct to use.

I'd really found not a single one in 20 years of experience in C, C++.
I found sometimes may reasons to use goto in fortran and basic, but
not a single one in C.

No, I dont like while or for only to avoid a goto! Such constructs are
always only needed when the design of the function is bad and
embryonic.

Ah, but what if there is no chance for "failure" in a given nested
loop? What's the point of returning an "error/successcode" then?

Where is the source for a goto then?

 

[The Real OS/2 Guy]

Between your first days of programming and today, did you learn anything
else? Do the rules that you learnt as a beginner still apply to you, or
have you grown since then?

Yes, yes and I've grown in many, many ways.

I'd found many better ways to design a program or function. I'd
learned many tricks to get better, more efficient code and how to
write that more readable too.

Even as I learned that usage is global variables is evil I won a fight
in using them instead of pup them through endless levels as
parameters.

It was a question of performance! Pumping many parameters through deep
levels of functions costs significantly more time than simply use the
values as global - and as runtime was the most critical point globals
were the solution.

I had fighted for the contrary if it were a normal application, but
the kernel of an OS requires other methods.