Error returns and repeated code, GOTOs, etc.

[Edward A. Falk]

On 05/30/2012 06:44 AM, mike3 wrote:

ErrorCode MyFunc()
{
BigNum a = BIGNUM_NULL;
BigNum b = BIGNUM_NULL;
BigNum c = BIGNUM_NULL;
BigNum d = BIGNUM_NULL;
ErrorCode rv;

rv = BigNum_Initialize(&a);
if(rv != ERROR_SUCCESS)
goto failed;

...

failed: /* we'll try and clean up, but won't care too much if it fails */
if (d != BIGNUM_NULL)
BigNum_Free(&d);
...

This is the style I tend to prefer. I've written a lot of device drivers
in the past, and the attach function generally has to catch about a
dozen failure cases. This is pretty much the cleanest & safest way
to deal with it.

 

[Ike Naar]

The /first/
compound statement starts at the first brace in your code.

A real quiche-eater would call that the zero-th compound statement ;-)

 

[James Kuyper]

A real quiche-eater would call that the zero-th compound statement ;-)

Such people really do exist; I once had a very confusing discussion with
someone who insisted on misusing English that way.

 

[Stefan Ram]

Such people really do exist; I once had a very confusing discussion with
someone who insisted on misusing English that way.

See also

http://www.purl.org/stefan_ram/pub/zero

 

[Joe keane]

I think he's talking about what he's seen in his experience, not a
definition

I think i'm talking about what i've seen in my experience, not a
definition

-- that he's tended to see postconditions of the form (a || b)

i tended to see all postconditions of the form

IF [error code is zero] THEN
all operations we did completed successfuly
go ahead with what you wanted to do
ELSE /* error code is not zero */
at least one thing we tried did not complete successfully
we tried to clean it up but we can not guarantee that
we tried to return the error code for the -first- error
if you fix the first error we can probably do a better job
at telling you what if anything else is wrong
ENDIF

where 'a' means "the postcondition you'd expect from a naive statement
of the algorithm" and 'b' means "some strange kludge that came about as
a result of an error in the course of executing the code".

; handling 'double errors' you will never get -right-, but at least you can
try to be helpful.

 

[mike3]

On May 29, 11:44 pm, mike3 <[email protected]> wrote:

<snip>

I've been wondering about this. _Why_ was a GOTO used in this
program? How could one be used in _such a high-profile program
like that_? And it's not just a lone GOTO or a few here and there,
as I said, this program is infected with a serious case of GOTOitis...

 

[Ben Bacarisse]

<snip>

I've been wondering about this. _Why_ was a GOTO used in this
program? How could one be used in _such a high-profile program
like that_? And it's not just a lone GOTO or a few here and there,
as I said, this program is infected with a serious case of GOTOitis...

You'd have to ask the people who wrote it.

Some of the code looks extraordinary to me. Whilst the vast majority of
goto's are clearly of the "error cleanup" sort, they are not all like
that. For example there is this pattern:

cmsgp = CMSG_FIRSTHDR(msg);
while (cmsgp != NULL) {
socket_log(...);

#ifdef ISC_PLATFORM_HAVEIN6PKTINFO
if (...) {
/* stuff */
goto next;
}
#endif

#ifdef SO_TIMESTAMP
if (...) {
/* stuff */
goto next;
}
#endif
next:
cmsgp = CMSG_NXTHDR(msg, cmsgp);
}

Why is this not just a for loop? Failing that, why not just remove the
label and the goto's? Maybe this is a fossilised relic of some much
more complex code.

Although there may well be a reason the code ended up like this, I hope
everyone agrees that it would be odd (to say the least) to write it like
this from scratch.

 

[Angel]

<snip>

I've been wondering about this. _Why_ was a GOTO used in this
program? How could one be used in _such a high-profile program
like that_? And it's not just a lone GOTO or a few here and there,
as I said, this program is infected with a serious case of GOTOitis...

A quick search through the source code of Linux 3.2.12 reveals no less
than 94422 goto statements.

I agree that one should think very carefully about using a goto statement
where structured code would be much better. But there are, especially in
exception handling (which, um, C doesn't really provide), cases where the
use of a goto statement produces much more elegant code.

There is an interesting read about it all here:
http://kerneltrap.org/node/553/2131


I agree with what Scott Robert Ladd states: Bad code is produced by bad
programmers, not by bad language features.

 

[Willem]

pete wrote:
) I recall many many years ago,
) showing some crappy looking code to my new boss,
) and him telling me that because of time constraints
) I would soon be writing crappy looking code too.
) And he was right.
) They hired me when they needed a programmer right away,
) and I didn't know C or good writing style
) nearly as well as I do now,
) and because of time constraints, I had to program quickly.

Your boss obviously was unaware that crappy code saves time in the
short run, but costs more time (with compound interest) in the long run.

But it seems that choosing short term benefits over long term benefits
is all the rage these days, so that is no surprise.


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

 

[Rui Maciel]

Your boss obviously was unaware that crappy code saves time in the
short run, but costs more time (with compound interest) in the long run.

But it seems that choosing short term benefits over long term benefits
is all the rage these days, so that is no surprise.

If someone is in the business of fixing problems, writing crappy code saves
time in the short run and guarantees a bit of a cash flow in the long run.

http://dilbert.com/strips/comic/1995-11-13/


Rui Maciel

 

[James Kuyper]

On 06/06/2012 09:49 AM, Willem wrote:
....

But it seems that choosing short term benefits over long term benefits
is all the rage these days, so that is no surprise.

"these days"? Has it ever not been so?

 

[gwowen]

ErrorCode MyFunc()
{
  BigNum a, b, c, d;
  ErrorCode rv = ERROR_SUCCESS;

BigNum * const vars[] = {&a,&b,&c,&d};
int inits;

// Try to initialise our variables
for(inits = 0; inits < 4 && rv == ERROR_SUCCESS; ++inits){
rv = BigNum_Initialize(vars[inits]);
}

// On success, do maths
if(rv == ERROR_SUCCESS) {
// Do some maths
}

// Unwind the stack of successful initialisations
for(; inits > 0; --inits){
BigNum_Free(vars[inits-1]);
}
return rv;
}

 

[mike3]

A quick search through the source code of Linux 3.2.12 reveals no less
than 94422 goto statements.

I agree that one should think very carefully about using a goto statement
where structured code would be much better. But there are, especially in
exception handling (which, um, C doesn't really provide), cases where the
use of a goto statement produces much more elegant code.

There is an interesting read about it all here:http://kerneltrap.org/node/553/2131

I agree with what Scott Robert Ladd states: Bad code is produced by bad
programmers, not by bad language features.

So would you say that using GOTO in the program I originally asked
about to
handle the errors would be an "acceptable use"?

 

[Tim Rentsch]

Edsgar Dijkstra is a quiche eater:
http://powerdown.free.fr/rp.html

"Was", not "is". He died in 2002.

To get a flavour of what a real "quiche eater" is like, read his
development of a program to find the convex hull of a set of 3D points,
which is done in parallel with the development of a proof of the
algorithm's correctness.[1] In fact, read the whole book, if you have
time. It changed the way I think about programming.

[1] E. W. Dijkstra, "A Discipline of Programming", Prentice Hall, 1977,
Chapter 24, pp 168--191.

An excellent book! A little difficult to read on
one's own, I think. I was lucky enough to get it
almost direct from the horse's mouth, in a course
taught by Martin Rem (who had been one of Dijkstra's
doctoral students). I count this as one of my three
main influences in program development.

 

[Angel]

So would you say that using GOTO in the program I originally asked
about to
handle the errors would be an "acceptable use"?

Since I haven't looked at the original program you refer to, I cannot
say really. My only point here is that not all uses of goto are
automatically "evil", and that there are real-life situations where its
use is justified and leads to better code.

 

[mike3]

On 2012-06-07, mike3 <[email protected]> wrote:

Since I haven't looked at the original program you refer to, I cannot
say really. My only point here is that not all uses of goto are
automatically "evil", and that there are real-life situations where its
use is justified and leads to better code.

I was referring to the example I showed in my OP:

http://groups.google.com/group/comp.lang.c/msg/338e8ac90f710e5e?hl=en&dmode=source

 

[Angel]

I was referring to the example I showed in my OP:

http://groups.google.com/group/comp.lang.c/msg/338e8ac90f710e5e?hl=en&
dmode=source

Perhaps it's not for me to say, as I haven't programmed seriously in
years. But I think this is exactly the same argument that the Linux
kernel people made for their use of "goto". So no, I personally don't
think the use of "goto" is bad in your example.

C has many features that make those who love structured code cry. But
IMHO, it's not the language's duty to protect bad programmers from
themselves. It's a tool, and it's up to the programmer how that tool
gets used.

 

[BartC]

I was referring to the example I showed in my OP:

http://groups.google.com/group/comp.lang.c/msg/338e8ac90f710e5e?hl=en&dmode=source

If you take the function with the gotos, and simplify the structure, it
looks like this:

Init(a); if(x) goto f0;
Init(b); if(x) goto f1;
Init(c); if(x) goto f2;
Init(d); if(x) goto f3;

Set(b,3); if(x) goto f4;

Free(d); if(x) goto f3;
Free(c); if(x) goto f2;
Free(b); if(x) goto f1;
Free(a); if(x) goto f0;
return(p);

f4: Free(d);
f3: Free(c);
f2: Free(b);
f1: Free(a);
f0:
return(q);

'x' is an error condition. In this form, you can see it is completely
dominated by gotos. And not in a simple pattern either. In fact, I would say
this qualifies as spaghetti code, even though all the gotos jump forward.
This could almost be some 1970s Basic code!

And there's something not quite right about this code. But apart from
anything else, there is a lot of repetition here: many lines almost
identical to each other. Perhaps some sort of loop is called for.

Also notice that the first block of Free() is error-checked; the second
block isn't; why not? Free(d) fails, you then Free(c), but don't seem to
care if that is OK or not. In that case, why bother checking the first lot?
Again, the two blocks do almost the same thing. (Imagine 26 variables
instead of 4, then it's clear this approach doesn't scale well.)

Even in this abbreviated form, with only four variables, and a single-line
main body, it's difficult to see what needs freeing, and what doesn't. This
has been discussed before: it's easier to arrange things so that you can
free an object unconditionally.

Or perhaps make use of variable-argument functions (I've never used them
myself), then you can have:

rv=MultiInit(a,b,c,d,...) /* stops at first error */

rv=MultiFree(a,b,c,d,....); /* returns first error, but frees the rest */

 

[Jean-Christophe]

(...) Or perhaps make use of variable-argument functions
(I've never used them myself), then you can have:
rv=MultiInit(a,b,c,d,...) /* stops at first error */
rv=MultiFree(a,b,c,d,....); /* returns first error, but frees the rest */

Still, if there are a lot of parameters (aa,ab,ac,...,zz)
then wouldn't it be better to init/free by using a loop ?
Assuming that a,b,c are different types of data structures,
it would make sense to use some struct to implement a void* pointer
to the data structures and pointers to their init/free functions,
initializing data w/ their pointers in a loop and stop at first error,
free data in a loop : don't need any goto, makes code shorter and more
clear.

 

[Malcolm McLean]

בת×ריך ×™×•× ×©×‘×ª,9 ביוני 2012 01:14:43 UTC+1, מ×ת Bart:

If you take the function with the gotos, and simplify the structure, it
looks like this:

Init(a); if(x) goto f0;
Init(b); if(x) goto f1;
Init(c); if(x) goto f2;
Init(d); if(x) goto f3;

Set(b,3); if(x) goto f4;

Free(d); if(x) goto f3;
Free(c); if(x) goto f2;
Free(b); if(x) goto f1;
Free(a); if(x) goto f0;
return(p);

f4: Free(d);
f3: Free(c);
f2: Free(b);
f1: Free(a);
f0:
return(q);

'x' is an error condition. In this form, you can see it is completely
dominated by gotos. And not in a simple pattern either. In fact, I would say
this qualifies as spaghetti code,

The central mistake is that the the consturctor takes an empty structure tofill in, rather than returning a pointer to an allocated structure. Then the destructor itself can return an error.
Change this, and you get

Init(a); if(x) goto f0;
Init(b); if(x) goto f0;
Init(c); if(x) goto f0;
Init(d); if(x) goto f0;

Set(b,3); if(x) goto f0;

Free(d);
Free(c);
Free(b);
Free(a);
return(p);

f0: Free(d);
Free(c);
Free(b);
Free(a);
return(q);

That's much mpre acceptable. The control flow is still doiminated by gotos,but that's because all the calls can return errors, which go into a singleerror handler.