slightly OT: error handling conventions
Matt Garman
function while avoiding memory leaks at the same time.
I write a lot of functions that end up looking like the following:
int my_func() {
char *a, *b;
int x;
a = malloc(some_number);
x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}
x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}
free(a);
free(b);
return NO_ERROR;
}
As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.
Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?
Thanks,
Matt
--
Please see the following for email info:
http://raw-sewage.net/index.php?file=email
Eric Sosman
`goto' is a possibility. Nested `if' plus a variable to
keep track of the return status is another:
int my_func(void) {
char *a, *b;
int result = ERROR;
a = malloc(some_number);
if (a != NULL) { // a test you omitted
x = another_function(a);
if (x != ERROR_CONDITION) {
x = func_that_mallocs(&b);
if (x != ERROR_CONDITION) {
do_something(); // another omission
result = NO_ERROR;
}
free (b); // dubious, but mimics your code
}
free (a);
}
return result;
}
... but this can become unwieldy. If there are a lot of things
to test, `goto' may be a good deal clearer:
int my_func(void) {
char *a = NULL, *b = NULL;
int result = ERROR;
a = malloc(some_number);
if (a == NULL) goto EXIT;
x = another_function(a);
if (x == ERROR_CONDITION) goto EXIT;
x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) goto EXIT;
do_something();
result = NO_ERROR;
EXIT:
free (b);
free (a);
return result;
}
Note that the initializations of `a' and `b' allow you to have
just a single exit-point instead of many.
If a function has very many such conditions, consider whether
it might make more sense to break it up into multiple smaller
functions, each able to clean up after itself and "pass the buck"
to the caller.
And then, of course, there's always `abort()' ...
Sheldon Simms
> I write a lot of functions that end up looking like the following:
>
> int my_func() {
> char *a, *b;
> int x;
>
> a = malloc(some_number);
Check to see that malloc succeeded
>
> x = another_function(a);
> if (x == ERROR_CONDITION) {
> free(a);
> return ERROR;
> }
>
> x = func_that_mallocs(&b);
> if (x == ERROR_CONDITION) {
> free(a);
> free(b);
> return ERROR;
> }
>
> free(a);
> free(b);
>
> return NO_ERROR;
> }
>
> As you can see, I essentially have a lot of redundant code.
<snip>
> Is there a general strategy for dealing with this kind of scenario?
> It seems like a goto might be useful here. Are there any other "nice"
> conventions (readable, maintainable, etc)?
I think using goto is a reasonable (readable, maintainable, etc.)
alternative for situations like this. However, I would make sure that
func_that_mallocs() cleans up after itself if it fails instead of
requiring its caller to do it.
-Sheldon
Ian Woods
news:slrnbp8lp4...@news-proxy.cso.uiuc.edu:
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;
}
There are plenty of ways this can be improved, but I find this particular
method to be easy to code and easy to analyse. I wouldn't expect most
people to just 'see' what was going on, so it's likely to be a pain for
someone else to maintain - at least until they're familiar with the idea.
Ian Woods
Jack Klein
<gar...@raw-sewage.bogus> wrote in comp.lang.c:
I may get blasted for this, but I've always liked this one:
int my_func()
{
char *a = NULL;
char *b = NULL;
FILE *fin = NULL;
FILE *fout = NULL;
int x = SUCCESS:
do
{
a = malloc(...);
b = malloc(...);
if (!a || !b)
{
x = OUT_OF_MEMORY;
break;
}
fin = fopen(...);
fout = fopen(...);
if (!fin || !fout)
{
x = FILE_OPEN_ERROR;
break;
}
/* do the real work */
} while (0);
free(a);
free(b);
if (fin) fclose(fin);
if (fout) fclose(fout);
return x;
}
Of course the do { ... } while (0) with breaks is just a disguised
goto, as indeed are all flow control structures, but inside a block
like this you know they all go to the same place, you don't have to
mentally check the goto labels to see if they are all the same.
As an aside, I have often wished that fclose(NULL) was defined as a
no-op, as is free(NULL), for the same reason of symmetry. Then you
could always fclose() the pointer returned by fopen() whether
successful or not, just as you can always free() the pointer returned
by *alloc(), successful or not.
--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://www.eskimo.com/~scs/C-faq/top.html
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++ ftp://snurse-l.org/pub/acllc-c++/faq
CBFalconer
I would write that as:
int my_func()
{
char *a, *b;
int x;
int err;
a = b = NULL;
if (!(a = malloc(some_number))) /* you omitted this test */
err = NOMEM;
else if (ERROR_CONDITION == (x = another_function(a)))
err = ERROR;
else if (ERROR_CONDITION == (x = func_that_mallocs(&b)))
err = ERROR;
else
err = NO_ERROR;
free(a);
free(b)
return err;
}
--
Chuck F (cbfal...@yahoo.com) (cbfal...@worldnet.att.net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home.att.net> USE worldnet address!
Richard Heathfield
8> 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 */
>
<snip>
>
> on_err:
> switch(rl) {
> case 3: free(mem);
> case 2: fclose(outf);
> case 1: fclose(inf);
> case 0: /* nothing to clean up */
> }
>
> return rv;
> }
>
> There are plenty of ways this can be improved, but I find this particular
> method to be easy to code and easy to analyse. I wouldn't expect most
> people to just 'see' what was going on, so it's likely to be a pain for
> someone else to maintain - at least until they're familiar with the idea.
I can certainly see what's going on, and I find your approach interesting,
but I do have a question. I know you're not a fan of spaghetti programming.
Here, though, there is little spaghetti to speak of, since you use goto to
jump to the end of the function (straight to the error-handling, that is).
This is the only use of goto I've ever seriously contemplated in C (because
it's not exactly spaghetti - or rather, it's more like rigid, uncooked
spaghetti, straight off the tree), but in fact I've always found other ways
to do it.
My question is this: Have you /considered/ other possibilities, especially
w.r.t. maintenance issues, or are you content with the goto in this
context? That is, have you explored this runlevel idea, /sans/ goto? If so,
what were your findings?
[Personal aside: Ian, I haven't forgotten, honest! I hope to get my email
fixed up any month now.]
--
Richard Heathfield : bin...@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton
Richard Heathfield
> I was wondering what conventions people use for error handling in a
> function while avoiding memory leaks at the same time.
>
> I write a lot of functions that end up looking like the following:
>
> int my_func() {
> char *a, *b;
> int x;
>
> a = malloc(some_number);
>
> x = another_function(a);
> if (x == ERROR_CONDITION) {
> free(a);
> return ERROR;
> }
>
> x = func_that_mallocs(&b);
> if (x == ERROR_CONDITION) {
> free(a);
> free(b);
> return ERROR;
> }
>
> free(a);
> free(b);
>
> return NO_ERROR;
> }
>
I've rewritten this in "my" style. See what you think.
int my_func(void)
{
int rc = SUCCESS;
char *a = malloc(some_number);
char *b = NULL;
if(a != NULL)
{
rc = another_function(a);
if(SUCCESS == rc)
{
rc = func_that_mallocs(&b);
if(SUCCESS == rc)
{
rc = DoStuff(a, b);
free(b);
}
}
free(a);
}
return rc;
Sheldon Simms
> My question is this: Have you /considered/ other possibilities, especially
> w.r.t. maintenance issues, or are you content with the goto in this
> context? That is, have you explored this runlevel idea, /sans/ goto? If so,
> what were your findings?
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?
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.
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.
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.
-Sheldon
Christian Bau
Sheldon Simms <sheldo...@yahoo.com> wrote:
> 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.
>
> 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.
If anybody asks for advice whether to use goto or not, you should tell
them not to use it. People who ask for advice shouldn't use goto.
At some point they will not ask for advice anymore, and at some point
they will either start thinking for themselves or they will not. If they
think for themselves, they will use goto when it is appropriate and
avoid it when it is inappropriate, and if they are smart they will know
when it is appropriate. If they don't think for themselves, they will
still insist that goto is always evil.
Usual indication of "not thinking" is when you present an example that
is more readable because it uses goto, and they insist that it is worse
because it uses goto. So the conclusion that they draw is that using
"goto" is bad because you use "goto".
Nick Keighley
"goto considered harmful"
http://www.acm.org/classics/oct95/
"structured programming with goto"
http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf
then google "heathfield goto addict"
(and Richard, don't let this goto your head...)
--
Nick Keighley
"I've been on the wagon now for more than a decade.
Not a single goto in all that time. I just don't
need them any more. I don't even use break or continue
now, except on social occasions of course.
And I don't get carried away."
Dan Pop
>It seems like a goto might be useful here.
A goto is useful any time it helps simplifying the program structure,
especially reducing the number of indentation levels (but not at the cost
of increasing the complexity of the program control structure, i.e.
generating spaghetti code).
Premature function termination (when cleanup is needed) is a typical case
of goto usage.
Dan
--
Dan Pop
DESY Zeuthen, RZ group
Email: Dan...@ifh.de
David Rubin
>
> I was wondering what conventions people use for error handling in a
> function while avoiding memory leaks at the same time.
>
> I write a lot of functions that end up looking like the following:
>
> int my_func() {
> char *a, *b;
> int x;
> a = malloc(some_number);
> x = another_function(a);
> if (x == ERROR_CONDITION) {
> free(a);
> return ERROR;
> }
> x = func_that_mallocs(&b);
> if (x == ERROR_CONDITION) {
> free(a);
> free(b);
> return ERROR;
> }
> free(a);
> free(b);
> return NO_ERROR;
> }
Here's one more refactored implementation:
int
my_func(void)
{
char *a = 0;
char *b = 0;
int r = NO_ERROR;
if((a=malloc(some_number)) == 0){
return ERROR;
}
if(another_function(a) == ERROR_CONDITION
|| func_that_mallocs(&b) == ERROR_CONDITION){
r = ERROR;
}
free(a);
free(b);
return r;
}
--
Andre, a simple peasant, had only one thing on his mind as he crept
along the East wall: 'Andre, creep... Andre, creep... Andre, creep.'
-- unknown
Erik Domstad
> I've rewritten this in "my" style. See what you think.
>
> int my_func(void)
> {
> int rc = SUCCESS;
> char *a = malloc(some_number);
> char *b = NULL;
>
> if(a != NULL)
> {
> rc = another_function(a);
> if(SUCCESS == rc)
> {
> rc = func_that_mallocs(&b);
> if(SUCCESS == rc)
> {
> rc = DoStuff(a, b);
> free(b);
> }
> }
> free(a);
> }
> return rc;
> }
I often write like this. It involves a few redundant tests of rc
but it is not nested as deep. I even check rc the first time even though
I know it's SUCCESS. This way it's easy to uncomment or move blocks of code.
Perhaps not very elegant but I think it's quite readable and easy to maintain.
int my_func(void)
{
int rc = SUCCESS;
char *a = NULL;
char *b = NULL;
if (SUCCESS == rc) {
a = malloc(some_number)
if (NULL == a)
rc = FAILURE;
}
if(SUCCESS == rc)
rc = another_function(a);
if(SUCCESS == rc)
rc = func_that_mallocs(&b);
if(SUCCESS == rc)
rc = DoStuff(a, b);
if (NULL != b)
free(b);
if (NULL != a)
free(a);
return rc;
}
Ed Morton
Dan Pop wrote:
> In <slrnbp8lp4...@news-proxy.cso.uiuc.edu> Matt Garman <gar...@raw-sewage.bogus> writes:
>
>
>>It seems like a goto might be useful here.
>
>
> A goto is useful any time it helps simplifying the program structure,
I agree in theory, but I've yet to see an example that wouldn't be more
easily understandable using variables. e.g. elsethread a program
structure like this was proposed (the only proposal I saw that used goto):
void foo()
int reached = 0;
if (a() == 3) {
goto cleanup;
} else {
reached++;
}
if (b() == 4) {
goto cleanup;
} else {
reached++;
}
if (c() < 0) {
goto cleanup;
} else {
reached++;
}
cleanup:
switch (reached) {
case 3: undoc();
case 2: undob();
case 1: undoa();
}
}
Yes, we could all figure out what this does, especially if we were
familiar with this style of coding, but why is a() returning 3
apparently an error? Ditto for b() returning 4? Again, if the values
tested for are obvious like NULL then again we can figure it out, but
why write this type of code forcing future developers enhancing or
debugging it to have to figure things out (i.e. synthesize the
abstraction) when you can write something like this in the first place:
void foo()
enum { INIT, GOT_RSRCS, SENT_MSG, GOT_RSP } rslt = INIT;
if (a() != 3) {
rslt = GOT_RSRCS;
}
if (rslt == GOT_RSRCS) {
if (b() != 4) {
rslt = SENT_MSG;
}
}
if (rslt == SENT_MSG) {
if (c() >= 0) {
rslt = GOT_RSP;
}
}
cleanup:
switch (rslt) {
case GOT_RSP: undoc();
case SENT_MSG: undob();
case GOT_RSRCS: undoa();
}
}
It's now immediately clear to next poor sap who has to work on this WHY
we're going to cleanup. Bear in mind that same poor sap could have
several hundred other lines of code to try to read and understand while
trying to fix a bug that's going to cause the loss of a
multi-million-dollar contract if not fixed by Friday!
Among others, I've read the Knuth and Djikstra articles about use of
gotos and I've yet to see an example of a reason to use them other than
a very slight increase in execution efficiency (which is, of course,
good enough reason in sme situations) but I'm open to persuasion.
Ed.
Sheldon Simms
> Sheldon Simms <sheldo...@yahoo.com> wrote in message news:<pan.2003.10.21....@yahoo.com>...
>> On Tue, 21 Oct 2003 06:05:24 +0000, Richard Heathfield wrote:
>
>> What would be the advantage of eliminating goto in a situation
>> like this?
>
> "goto considered harmful"
> http://www.acm.org/classics/oct95/
>
> "structured programming with goto"
> http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf
Yes yes, thanks. I've read them before.
But Dijkstra isn't God, and "Go To Considered Harmful" isn't
holy scripture. As for Knuth, he's *for* goto, as long as
it's properly used.
So once again, what would be the advantage of eliminating
goto in the code under discussion (which I reinsert below)?
> 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 */
>
Sheldon Simms
> Usual indication of "not thinking" is when you present an example that
> is more readable because it uses goto, and they insist that it is worse
> because it uses goto.
What they will insist is that the version without goto is more
readable. And the reason they will give for that is precisely that
there is no goto. Since "more readable" is a subjective judgement,
it's impossible to argue the point, so I don't try anymore.
Dan Pop
>
>Dan Pop wrote:
>
>> In <slrnbp8lp4...@news-proxy.cso.uiuc.edu> Matt Garman <gar...@raw-sewage.bogus> writes:
>>
>>
>>>It seems like a goto might be useful here.
>>
>>
>> A goto is useful any time it helps simplifying the program structure,
>
>I agree in theory, but I've yet to see an example that wouldn't be more
>easily understandable using variables. e.g. elsethread a program
>structure like this was proposed (the only proposal I saw that used goto):
>
>void foo()
Where is the brace?
Ditto.
> enum { INIT, GOT_RSRCS, SENT_MSG, GOT_RSP } rslt = INIT;
>
> if (a() != 3) {
> rslt = GOT_RSRCS;
> }
> if (rslt == GOT_RSRCS) {
> if (b() != 4) {
> rslt = SENT_MSG;
> }
> }
> if (rslt == SENT_MSG) {
> if (c() >= 0) {
> rslt = GOT_RSP;
> }
> }
>
>cleanup:
> switch (rslt) {
> case GOT_RSP: undoc();
> case SENT_MSG: undob();
> case GOT_RSRCS: undoa();
> }
>}
In many cases, it is relatively easy to determine what needs to be
undone without explicitly keeping track of it. This leads to the much
simpler:
void foo()
{
if (a() != OK) goto cleanup;
if (b() != OK) goto cleanup;
c();
cleanup:
if (donec) undoc();
if (doneb) undob();
if (donea) undoa();
}
Note that this is pseudocode, the done* things being expressions, rather
than variables and the undo* things being cleanup actions, rather than
actual function calls.
And given the triviality of the example, it can be rewritten without goto
and without nested if's:
a() == OK && b() == OK && c() ;
but this is not the point. I'd rather use the goto version, BTW ;-)
The things that usually need cleanup in a standard C program are
dynamically allocated buffers and open streams. If the corresponding
pointers are initialised to NULL, no check whatsoever is needed for
freeing the buffers, an unconditional free() call is doing the job,
only FILE pointers need to be checked and fclose() called if they are
non-null.
Similar tricks can be done for other resources that need to be released
by non-portable C programs, in order to simplify the overall code
structure. If all the gotos in a function have the same destination,
few people have a valid reason for complaining.
CBFalconer
>
... snip ...
>
> If anybody asks for advice whether to use goto or not, you should tell
> them not to use it. People who ask for advice shouldn't use goto.
Excellent. The last sentence is sig material.
CBFalconer
>
... snip ...
>
> I've rewritten this in "my" style. See what you think.
>
> int my_func(void)
> {
> int rc = SUCCESS;
> char *a = malloc(some_number);
> char *b = NULL;
>
> if(a != NULL)
> {
> rc = another_function(a);
> if(SUCCESS == rc)
> {
> rc = func_that_mallocs(&b);
> if(SUCCESS == rc)
You have a hidden assumption here, i.e. that failure does not
malloc(&b).
> {
> rc = DoStuff(a, b);
> free(b);
> }
> }
> free(a);
> }
> return rc;
> }
--
Richard Heathfield
> On Tue, 21 Oct 2003 06:05:24 +0000, Richard Heathfield wrote:
>
>> My question is this: Have you /considered/ other possibilities,
>> especially w.r.t. maintenance issues, or are you content with the goto in
>> this context? That is, have you explored this runlevel idea, /sans/ goto?
>> If so, what were your findings?
>
> 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
> Richard Heathfield wrote:
>>
>> rc = func_that_mallocs(&b);
>> if(SUCCESS == rc)
>
> You have a hidden assumption here, i.e. that failure does not
> malloc(&b).
I was guided entirely by the function name, which I took to be
self-documenting, as all good function names are. :-)
E. Robert Tisdale
> I may get blasted for this
You are so correct.
This is a good example of *very* poor programming practice.
>
> Of course the do { ... } while (0) with breaks is just a disguised
> goto, as indeed are all flow control structures, but inside a block
> like this you know they all go to the same place, you don't have to
> mentally check the goto labels to see if they are all the same.
>
> As an aside, I have often wished that fclose(NULL) was defined as a
> no-op, as is free(NULL), for the same reason of symmetry. Then you
> could always fclose() the pointer returned by fopen() whether
> successful or not, just as you can always free() the pointer returned
> by *alloc(), successful or not.
int my_func(void) { // returns NO_ERROR or ERROR
int error = N0_ERROR; // default return value
char* a = (char*)malloc(some_number);
if (NULL != a) {
int x = another_function(a);
if (ERROR_CONDITION == x) {
error = ERROR;
}
else { // ERROR_CONDITION != x
char* b = NULL; // define variables & constants
// close to first use.
int x = func_that_mallocs(&b);
if (ERROR_CONDITION == x) {
error = ERROR;
}
free(b); // free in same scope as malloc
}
}
free(a); // free in same scope as malloc
return error; // single exit point
}
1. C functions should have a single exit point.
2. Don't declare undefined variables.
3. Place veariable and constant definitions
as close to first use as possible.
4. Don't declare variables in an enclosing scope
just to "reuse" the memory allocated for them.
5. Free allocated memory as soon as possible.
Mark Gordon
> For code where I have multiple resources to find, use, then release I
I've used a switch for doing cleanup like this as well, although I used
the setting of an error code on failure to control the clean up. In my
case cleanup also included sending messages over a comms link.
You could get rid of the gotos by using a loop and break to escape on
error.
--
Mark Gordon
Paid to be a Geek & a Senior Software Developer
Although my email address says spamtrap, it is real and I read it.
Jirka Klaue
...
> This is a good example of *very* poor programming practice.
> 1. C functions should have a single exit point.
if ((f = fopen(fn, "r")) == 0) {
fprintf(stderr, "error opening %s\n", fn);
return 0;
}
Are you suggesting that the return statement
should be replaced by a goto? ;-)
Jirka
Christian Bau
Are you trolling?
Mindless obeyance to rules creates bad code, as you demonstrate
perfectly well.
Ed Morton
On 10/21/2003 11:52 AM, Dan Pop wrote:
> In <bn3jge$k...@netnews.proxy.lucent.com> Ed Morton <mortonAVO...@lucent.com> writes:
>
>
>
>>Dan Pop wrote:
>>
>>
>>>In <slrnbp8lp4...@news-proxy.cso.uiuc.edu> Matt Garman <gar...@raw-sewage.bogus> writes:
>>>
>>>
>>>
>>>>It seems like a goto might be useful here.
>>>
>>>
>>>A goto is useful any time it helps simplifying the program structure,
>>
>>I agree in theory, but I've yet to see an example that wouldn't be more
>>easily understandable using variables. e.g. elsethread a program
>>structure like this was proposed (the only proposal I saw that used goto):
>>
>>void foo()
>
>
> Where is the brace?
They were out of them at the store. Apparently it's the end of the season and I
just missed the clearance sale.
<snip>
> In many cases, it is relatively easy to determine what needs to be
> undone without explicitly keeping track of it. This leads to the much
> simpler:
>
> void foo()
> {
> if (a() != OK) goto cleanup;
> if (b() != OK) goto cleanup;
> c();
>
> cleanup:
> if (donec) undoc();
> if (doneb) undob();
> if (donea) undoa();
> }
>
> Note that this is pseudocode, the done* things being expressions, rather
> than variables and the undo* things being cleanup actions, rather than
> actual function calls.
OK, then how would you approach the OPs specific function? I'd expect your
answer to look like this:
int my_func() {
char *a, *b;
int x, ret = ERROR;
a = malloc(some_number);
if (a == NULL) goto cleanup;
x = another_function(a);
if (x == ERROR_CONDITION) goto cleanup;
x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) goto cleanup;
ret = NO_ERROR;
cleanup:
free(b);
free(a);
return ret;
}
but this in theory invokes undefined behavior if func_that_mallocs() is written
as (pseudo-code):
int func_that_mallocs(char **b) {
*b = malloc(...);
.... /* ensure b is not NULL and do other things */
if (today_is_tuesday) {
free(*b);
return ERROR_CONDITION;
} else {
return SUCCESS;
}
}
since at the end of my_func() you could be accessing a pointer (b) that's
already been free-ed. And that's just this special case where it's safe to call
the "dealloc" function (free()) when the "alloc" function (malloc()) fails.
So, I don't agree that it's usually easy to determine what needs to be undone
and, even when you can, it's often inefficient compared to having a specific
variable that TELLS you what needs to be undone and so gainsays the slight
performance advantage introduced by the goto.
> And given the triviality of the example, it can be rewritten without goto
> and without nested if's:
>
> a() == OK && b() == OK && c() ;
>
> but this is not the point. I'd rather use the goto version, BTW ;-)
I'd guess we may be about one or 2 postings away from "let's agree to disagree" ;-).
Don't get me wrong, I do think that using gotos can improve the readability of
convoluted code, I just think that if you go into it assuming that you'll use
gotos then you'll miss an opportunity to do an even better job. 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;
}
which isn't significantly longer or more highly indented than the goto-version
but doesn't have the UB problem and would work in the general case of any
resource that can only have it's "dealloc" function called if it's associated
"alloc" had succeeded.
Regards,
Ed.
Richard Heathfield
Disclaimer: It's not my fault if Tisdale happens to agree with something
I've said many times in the past.
I don't think a goto is necessary. I would suggest that the code /could/ be
replaced by something along these lines:
fp = fopen(fn, "r");
if(fp != NULL)
{
rc = foo(fp);
fclose(fp);
}
else
{
rc = CANNOT_OPEN_FILE;
}
return rc;
Christian Bau
Ed Morton <mortonAVO...@Lucent.com> wrote:
> 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;
> }
>
> which isn't significantly longer or more highly indented than the
> goto-version
> but doesn't have the UB problem and would work in the general case of any
> resource that can only have it's "dealloc" function called if it's associated
> "alloc" had succeeded.
Introducing an enum and a switch statement is completely unnecessary and
only complicates matters for no gain in readability at all. Consider
this version instead:
int my_func() {
char *a, *b;
int x, ret = ERROR;
a = malloc(some_number);
if (a == NULL) goto FAILED_A_MEM;
x = another_function(a);
if (x == ERROR_CONDITION) goto FAILED_A;
x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) goto FAILED_B_MEM;
ret = NO_ERROR;
free(b);
FAILED_B_MEM:
FAILED_A:
free(a);
FAILED_A_MEM:
return ret;
}
Nick Keighley
> On Tue, 21 Oct 2003 04:08:42 -0700, Nick Keighley wrote:
> > Sheldon Simms <sheldo...@yahoo.com> wrote in message
> > news:<pan.2003.10.21....@yahoo.com>...
> >> On Tue, 21 Oct 2003 06:05:24 +0000, Richard Heathfield wrote:
> >> What would be the advantage of eliminating goto in a situation
> >> like this?
> >
> > "goto considered harmful"
> > http://www.acm.org/classics/oct95/
> >
> > "structured programming with goto"
> > http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf
>
> Yes yes, thanks. I've read them before.
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. And he puts the case better than
anything else I've read. Though you have to put it in context; when he
said it, it was arguing against a more pro-goto culture.
> But Dijkstra isn't God, and "Go To Considered Harmful" isn't
> holy scripture.
blasphemy!
> As for Knuth, he's *for* goto, as long as
> it's properly used.
that's why I posted the link. The point (as others in this thread have
said) is "properly used".
I have no burning urge to strip out the gotos. In fact I quite like
the
code.
--
Nick Keighley
I mean, if 10 years from now, when you are doing something quick and
dirty,
you suddenly visualize that I am looking over your shoulders and say
to
yourself, "Dijkstra would not have liked this", well that would be
enough
immortality for me.
-- Edsger W Dijkstra
Jirka Klaue
> Jirka Klaue wrote:
>>E. Robert Tisdale wrote:
>>
>>>1. C functions should have a single exit point.
>>
>> if ((f = fopen(fn, "r")) == 0) {
>> fprintf(stderr, "error opening %s\n", fn);
>> return 0;
>> }
>>
>>Are you suggesting that the return statement
>>should be replaced by a goto? ;-)
>
> replaced by something along these lines:
>
> fp = fopen(fn, "r");
> if(fp != NULL)
> {
> rc = foo(fp);
> fclose(fp);
> }
> else
> {
> rc = CANNOT_OPEN_FILE;
> }
>
> return rc;
> }
IMO this method has two drawbacks. It leads to functions doing
little with *many* lines. In case of more preparation (more files,
some mallocs, sockets, whatever) it gets messy pretty quick. The
method does not scale very good.
Jirka
PS: Didn't you promise, you will not get involved in a style discussion
anymore? ;-)
Ed Morton
Christian Bau wrote:
<snip>
>
> Introducing an enum and a switch statement is completely unnecessary and
> only complicates matters for no gain in readability at all.
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.
Consider
> this version instead:
>
> int my_func() {
> char *a, *b;
> int x, ret = ERROR;
>
> a = malloc(some_number);
> if (a == NULL) goto FAILED_A_MEM;
>
> x = another_function(a);
> if (x == ERROR_CONDITION) goto FAILED_A;
>
> x = func_that_mallocs(&b);
> if (x == ERROR_CONDITION) goto FAILED_B_MEM;
>
> ret = NO_ERROR;
>
> free(b);
> FAILED_B_MEM:
> FAILED_A:
> free(a);
> FAILED_A_MEM:
> return ret;
> }
Yeah, I thought about that too, but as Dan mentioned:
>>Dan Pop wrote:
<snip>
> If all the gotos in a function have the same destination,
> few people have a valid reason for complaining.
>
In the code you just posted, the gotos all have different destinations
so I didn't want to suggest that that's the type of code Dan would write
given his explicit desire to avoid doing that.
What if the function had 10 failing conditions/cleanups? Would you
really introduce 10 goto destinations?
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.
Even regardless of readability, think about debugging your version
versus mine. In mine, at the end of the function you could just dump the
"rslt" variable and know how the execution went, whereas in yours .....
So, while it doesn't look too bad for this particular function, I don't
think that in general having multiple gotos is a better alternative to a
variable tracking progress.
Ed.
Jirka Klaue
> 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;
switch (rc) {
A: release(x); break;
B: release(x);
C: release(y);
}
The Real OS/2 Guy
<christ...@cbau.freeserve.co.uk> wrote:
some non-transparent code.
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.
Good design in programming level is:
- a function has only one return in either success or fail
The same function can have many return in the contrary case.
That means: when the natural result of a function is FAIL
then only ONE exit that flags the fail exist, but multiple
returns flagging success may exist anyway.
When the natural meaning of a function is success then only
ONE return that flags the success exist, but multiple
return flagging an error can exist.
- document each function well. This is needed for maintenance anyway,
even when you are the only for eternity who will own the source.
This will save you moth of maintenance.
document the interface of each funtion well. This will
save you time in using it.
- a fuction exeeding substantial more than one screen page
is buggy per design.
- nesting more than 3 blocks (conditional, loops) tends to be
errornous
break them down in nested fuctions increases the readability.
- breaking nested loops into nested functions helps in readability
because a return error/successcode says more than a goto can ever
say.
- whenever your compiler is unable to support _inline_ use the
preprocessor to define micro steps who are in themself not clearly
writeable, because a self speaking function/macro name tells more
than a non-transparent statement ever can. This increases the
maintenanceability significantly.
- define macros who are object of changeability globally
- define macros you creates only for increasing the readability of
code near the place(s) you need them. Don't forged to #undef them.
- avoid casting. Recheck the design before you tries to cast!
cast only when you absolutely and exactly knows what you are doing,
cast never only to get the comiler quiet without knowing
exactly why it whines and you're not 100% sure that you knows what
you're doing is better than the comiler knows. Be sure, commonly
the compiler knows better than you why it is crying.
Whenever a fuction tends to be bigger than a screen page (about 50
lines), break it down in multiple functions:
- describe WHAT is to do in higher level and HOW is it done in lower
level
- break WHAT is to do from overview to more detailed.
- break HOW is to do to WHAT is to do until you gets fine details
- break HOW is to do in more fine details
- don't nest loops with conditions inside - make own functions out of
them.
This increases the readability and makes it easier to debug
Whereas details means ownstanding functions. It is noways a disgrace
to write a function that gets called once. Because that function helps
you to write readable, clean code as it hides details in the flow and
tells by its name what is to be done.
A side effect of the above would be that you avoids goto, increases
the readability and maintenanceability significantly.
Again: prefere design over coding! Code only when the design is
crlean, checked and rechecked.
At compile time use the most detailed warning level - always! Because
the compier knows more about C as you'll ever be learn when you're not
a compiler developer. Letz the compiler help you to find unsave,
unsecure, unstable code! Again: never suppress a warning by casting
except you have checked and rechecked that you exactly knows that you
knows that the compiler can't know something you knows and you knows
here that even as the compiler means you may loose some data you knows
that it is impossible because you knows the min/max occures here fits.
--
Tschau/Bye
Herbert
eComStation 1.1 Deutsch wird jetzt ausgeliefert!
Ed Morton
Jirka Klaue wrote:
Sorry it wasn't clear - the A, B, and C in this case are goto labels.
That type of switch would work for the preceeding case where I was using
a variable to hold results though if you prefer that to the if
statements and don't mind duplicating the call to "release(x);".
Ed.
Jirka Klaue
> Jirka Klaue wrote:
>> Ed Morton wrote:
>>
>>> 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;
>>
>>
>> switch (rc) {
>> A: release(x); break;
>> B: release(x);
>> C: release(y);
>> }
>
> Sorry it wasn't clear - the A, B, and C in this case are goto labels.
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.
Jirka
Dan Pop
>OK, then how would you approach the OPs specific function? I'd expect your
>answer to look like this:
>
>int my_func() {
> char *a, *b;
At least b MUST be initialised as a null pointer, otherwise a failed
malloc call would result in free() being called on an uninitialised
pointer.
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.
CBFalconer
> Sheldon Simms <sheldo...@yahoo.com> wrote in message
> > On Tue, 21 Oct 2003 04:08:42 -0700, Nick Keighley wrote:
>
Once more, although I am an advocate of suitable use of goto, this
is not one of them. I would write:
int do_stuff(char * iname, char * oname)
{
int rv; /* the error return value */
FILE *inf;
FILE *outf = NULL;
void *mem = NULL;
if (!(inf = fopen(iname,"r")) rv = ERROR;
else if (!(outf = fopen(oname,"w"))) rv = ERROR;
else if (!(mem = malloc(WHATEVER_SIZE))) rv = ERROR;
else {
/* do stuff with inf, outf and mem */
rv = 0;
}
free(mem);
if (outf) fclose(outf);
if (inf) fclose(inf);
return rv;
}
which I believe has similar semantics, less code, fewer variables,
and more clarity. It does not have the memory leak, nor the
failure to close the files on success. It still lacks checking
for fclose success.
Sheldon Simms
> On Wed, 22 Oct 2003 07:06:27 UTC, Christian Bau
> <christ...@cbau.freeserve.co.uk> wrote:
>
> some non-transparent code.
Actually it was completely obvious what was going on there.
If you were confused by it, perhaps you should give up on
programming.
> I've learned in my first days of programming (the dark age where not
> even assembly was available) that
Oh really? You've been programming since before assembly
was available? When was that then, 1948?
> - making a well design is more than the half work.
> - goto is evil and should be avoided whenever possible
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.
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.
> And with C you can design the code always in a
> manner that NO goto is needed.
True, but there are also times in C programming when goto is
an appropriate construct to use.
> - a function has only one return in either success or fail
> The same function can have many return in the contrary case.
> That means: when the natural result of a function is FAIL
> then only ONE exit that flags the fail exist, but multiple
> returns flagging success may exist anyway.
> When the natural meaning of a function is success then only
> ONE return that flags the success exist, but multiple
> return flagging an error can exist.
Herbert, erbarme dich unser.
> - a fuction exeeding substantial more than one screen page
> is buggy per design.
Du nimmst hinweg die Sünde der Programmierer. Erbarme dich unser.
> - nesting more than 3 blocks (conditional, loops) tends to be
> errornous break them down in nested fuctions increases the
> readability.
A goto can work here too.
> - breaking nested loops into nested functions helps in
> readability because a return error/successcode says more than
> a goto can ever say.
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?
> Whenever a fuction tends to be bigger than a screen page (about 50
> lines), break it down in multiple functions: - describe WHAT is to do in
> higher level and HOW is it done in lower level - break WHAT is to do
> from overview to more detailed. - break HOW is to do to WHAT is to do
> until you gets fine details - break HOW is to do in more fine details -
> don't nest loops with conditions inside - make own functions out of
> them.
Herbert sei dank!
> This increases the readability and makes it easier to debug
> Whereas details means ownstanding functions. It is noways a disgrace to
> write a function that gets called once. Because that function helps you
> to write readable, clean code as it hides details in the flow and tells
> by its name what is to be done.
Das Evangelium nach Herbert
> A side effect of the above would be that you avoids goto, increases the
> readability and maintenanceability significantly.
Amen.
> Again: prefere design over coding! Code only when the design is crlean,
> checked and rechecked.
>
> At compile time use the most detailed warning level - always! Because
> the compier knows more about C as you'll ever be learn when you're not a
> compiler developer. Letz the compiler help you to find unsave, unsecure,
> unstable code! Again: never suppress a warning by casting except you
> have checked and rechecked that you exactly knows that you knows that
> the compiler can't know something you knows and you knows here that even
> as the compiler means you may loose some data you knows that it is
> impossible because you knows the min/max occures here fits.
Ehre sei dir, O Herbert.
Amen.
Ed Morton
Dan Pop wrote:
<snip>
> 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.
I think it's a pretty reasonable example - if you're wrapping malloc (or
any other resource-allocating primitive) in a function, presumably it's
doing more than just malloc-ing the memory, and if it returns failure
I'd hope it releases any resources it did succesfully grab before
returning. The only real question is whether or not it should
additionally set *b to NULL before return but:
a) although UNLIKELY to create a real problem, according to the FAQ
(http://www.eskimo.com/~scs/C-faq/q7.21.html), accessing a pointer after
free-ing it produces implementation-defined behavior, and
b) the only reason to do that is because the calling function now
depends on it, which is data-coupling and so to be avoided.
The value of b in
> my_func() MUST NOT be indeterminate after calling func_that_mallocs.
I'd say that the calling function MUST NOT make any assumptions about
the value of b when func_that_mallocs() returns failure. I'd also say
that free(NULL) is a special case anyway and that in general you
shouldn't assume you can unconditionally call the "dealloc()" function
if the "alloc()" one failed (as you pointed out with fclose()).
Ed.
Sheldon Simms
> Sheldon Simms <sheldo...@yahoo.com> wrote in message news:<pan.2003.10.21....@yahoo.com>...
>> On Tue, 21 Oct 2003 04:08:42 -0700, Nick Keighley wrote:
>> > Sheldon Simms <sheldo...@yahoo.com> wrote in message
>> > news:<pan.2003.10.21....@yahoo.com>...
>> >> On Tue, 21 Oct 2003 06:05:24 +0000, Richard Heathfield wrote:
>
>> >> What would be the advantage of eliminating goto in a situation
>> >> like this?
>> >
>> > "goto considered harmful"
>> > http://www.acm.org/classics/oct95/
>> >
>> > "structured programming with goto"
>> > http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf
>>
>> Yes yes, thanks. I've read them before.
>
> 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
wrote:
>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
wrote:
>
> The value of b in
>> my_func() MUST NOT be indeterminate after calling func_that_mallocs.
>
>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:
<snip>
> 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.
> exit:
> return rv;
> }
>
Ed Morton
rihad wrote:
> On Wed, 22 Oct 2003 12:03:28 -0500, Ed Morton <mortonAVO...@lucent.com>
> wrote:
>
>
>> The value of b in
>>
>>>my_func() MUST NOT be indeterminate after calling func_that_mallocs.
>>
>>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.
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.
>
>>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,
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
wrote:
>
>
>rihad wrote:
><snip>
>> 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?
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.
>
>> 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.
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.
>
>> /* 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.
>
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:
<snip>
> 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
Ed Morton <mortonAVO...@lucent.com> wrote:
> Christian Bau wrote:
> <snip>
> >
> > Introducing an enum and a switch statement is completely unnecessary and
> > only complicates matters for no gain in readability at all.
>
> 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
"The Real OS/2 Guy" <os2...@pc-rosenau.de> wrote:
> On Wed, 22 Oct 2003 07:06:27 UTC, Christian Bau
> <christ...@cbau.freeserve.co.uk> wrote:
>
> 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
Jirka Klaue <jkl...@ee.tu-berlin.de> wrote:
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
wrote:
> 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
Sheldon Simms <sheldo...@yahoo.com> wrote:
> 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
Ed Morton <mortonAVO...@lucent.com> wrote:
> > 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.
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
> Richard Heathfield wrote:
>>
>> I don't think a goto is necessary. I would suggest that the code /could/
>> be replaced by something along these lines:
>>
>> fp = fopen(fn, "r");
>> if(fp != NULL)
>> {
>> rc = foo(fp);
>> fclose(fp);
>> }
>> else
>> {
>> rc = CANNOT_OPEN_FILE;
>> }
>>
>> return rc;
>> }
>
> 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 : bin...@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton
Richard Heathfield
> On Tue, 21 Oct 2003 06:05:24 +0000, Richard Heathfield wrote:
>
>> My question is this: Have you /considered/ other possibilities,
>> especially w.r.t. maintenance issues, or are you content with the goto in
>> this context? That is, have you explored this runlevel idea, /sans/ goto?
>> If so, what were your findings?
>
> 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
wrote:
>
>
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.
--
Irrwahn
(irrw...@freenet.de)
The Real OS/2 Guy
<sheldo...@yahoo.com> wrote:
> On Wed, 22 Oct 2003 11:09:48 +0000, The Real OS/2 Guy wrote:
>
> > On Wed, 22 Oct 2003 07:06:27 UTC, Christian Bau
> > <christ...@cbau.freeserve.co.uk> wrote:
> >
> > some non-transparent code.
>
> 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.
> > I've learned in my first days of programming (the dark age where not
> > even assembly was available) that
>
> 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.
> > - making a well design is more than the half work.
> > - goto is evil and should be avoided whenever possible
>
> 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.
>
> > And with C you can design the code always in a
> > manner that NO goto is needed.
>
> 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
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.
Jirka Klaue
>Jirka Klaue wrote:
>>Richard Heathfield wrote:
>>
>>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
>
> So that's the line problem sorted.
Do you think so? And why did you remove all the other white space too?
I was not discussing formatting or indentation. I said, I prefer not
to write unnecessary lines (code), if lesser lines (code) suffice.
>>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.
Obviously.
>>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.
Okay, watch this. :-) <bllm7u$oiq$1...@titan.btinternet.com>
Jirka
rihad
<os2...@pc-rosenau.de> wrote:
>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
> (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...
... by writing code that emulates gotos. Of course you can get rid of gotos, by
introducing a level of abstraction over gotos (like extra logic etc etc). I
really don't understand you people, those who say that gotos are evil, but I'm
trying real hard :)
>
>Good design in programming level is:
>
>- a fuction exeeding substantial more than one screen page
> is buggy per design.
This is hilarious. Are we talking about 80x25 terminal emulation screen or
1600x1200 gvim in a maximized window? :)
Seriously, if what a function does can be considered atomic, if there are no
smaller useful subactions that could warrant making it out of that function into
functions of their own, then it can get as long as it gets. It doesn't happen
*usually*, but the function *may* get as long as it gets.
>- define macros you creates only for increasing the readability of
> code near the place(s) you need them. Don't forged to #undef them.
Agreed.
Default User
> I have some N-dimensional paper for sale. Only US$1000000 per dimension per
> sheet.
Are you charging the same for "rolled up" dimensions?
Brian Rodenborn
Dan Pop
>On Wed, 22 Oct 2003 11:09:48 +0000, The Real OS/2 Guy wrote:
>
>> I've learned in my first days of programming (the dark age where not
>> even assembly was available) that
>
>Oh really? You've been programming since before assembly
>was available? When was that then, 1948?
The availability of an assembler is strongly connected to the availability
of a hosted system capable of running it. As late as 1983, I had to
develop embedded control applications without the help of an assembler,
because the *only* computer available was the one I was programming for
and its resources (1K EPROM, 64 bytes RAM) were not enough for running
an assembler on.
By 1984 our department finally got a computer (a PDP-11 clone) and I
could write and use an assembler for the microcontroller in question.
Dan
--
Dan Pop
DESY Zeuthen, RZ group
Email: Dan...@ifh.de
Ian Woods
news:bn2ib3$afv$1...@titan.btinternet.com:
> Ian Woods wrote:
>
> 8> 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 */
>>
> <snip>
>>
>> on_err:
>> switch(rl) {
>> case 3: free(mem);
>> case 2: fclose(outf);
>> case 1: fclose(inf);
>> case 0: /* nothing to clean up */
>> }
>>
>> return rv;
>> }
>>
<snip>
> My question is this: Have you /considered/ other possibilities,
> especially w.r.t. maintenance issues, or are you content with the goto
> in this context? That is, have you explored this runlevel idea, /sans/
> goto? If so, what were your findings?
Using goto for this isn't the 'best' solution for most people, especially
with regards to maintenance. There are three considerations when making
code, or, like this is a code template:
o ease of writing
o ease of reading
o ease of maintenance
The goto approach is not the best at any of these, but it is easy to
write, easy to read /and/ easy to maintain.
It eventually grew out of the application of some of the techniques seen
elsethread. It is fact exactly equivalent to the nested-if technique you
posted. The big problem I have with that technique is that I find it
difficult to keep track of which allocation code is associated with which
cleanup code. When I have used that technique and had to revise the code,
I made the error of putting the 'right' code in the wrong place too often
for me to be comfortable.
The reason why it's goto which is used as opposed to something like break
in a do-loop is because of the kind of problems I'm typically solving.
It's common that I need to handle an error condition several loops in.
Either I need to write magic code to exit each loop, or I use goto. The
latter has some interesting uses for cleanup of the effects of nested
loops:
int foo(size_t w) {
int rl=0;
int rv=ERROR:
struct Object ** arr=NULL;
size_t x=0;
arr=malloc(w*sizeof *arr);
if (!arr) goto on_err;
for (x=0;x<w;x++) {
arr[x]=Object_Alloc();
if (!arr[x]) goto on_err;
}
rl++;
/* rl=1 - do stuff*/
rv=OKAY;
on_err:
switch(rl) {
case 1: x=w; /* so we can reuse x in rl 1 and higher... */
case 0: /* cleanup of arr */ {
size_t i;
for (i=0;i<x;i++) {
Object_DeAlloc(arr[i]);
}
free(arr);
}
}
return rv;
}
The goto is a one-size-fits-all solution to passing control to the
cleanup code. No additional code is needed to get to the cleanup stage as
with other things like breaking from within nested loops.
Summary: yes, I have considered other options, and the goto one is the
one I like 'best'. :)
> [Personal aside: Ian, I haven't forgotten, honest! I hope to get my
> email fixed up any month now.]
Don't worry - I've been trying hard so that the senior management don't
close my project (and funding!). Keeping them happy has meant that I've
had little time for sleep, never mind anything more interesting.
Ian Woods
Richard Heathfield
> Richard Heathfield wrote:
>>Jirka Klaue wrote:
>>>Richard Heathfield wrote:
>>>
>>>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
> [many lines snipped]
>>
>> So that's the line problem sorted.
>
> Do you think so?
No. But I didn't think there was a line problem to start with.
> And why did you remove all the other white space too?
To reduce the number of lines. ;-)
> I was not discussing formatting or indentation. I said, I prefer not
> to write unnecessary lines (code), if lesser lines (code) suffice.
I got 64 lines down to 17. I think that counts as fewer lines. %-)
>>>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.
>
> Obviously.
Well, in all seriousness, I use a terminal window that can easily display 50
lines in perfect clarity. You can see a fair bit of code in 50 lines, even
with extra braces and vertical whitespace and so on. I am currently looking
at the unmunged original of the code I posted in my previous article. I can
see almost the entire function on a single screen. If I used a slightly
smaller font, I could get the whole thing. I don't see a problem here. Yes,
if I used a terser style, I could see two functions at the same time. But I
don't want, or need, to see two functions at the same time!
>
>>>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.
>
> Okay, watch this. :-) <bllm7u$oiq$1...@titan.btinternet.com>
LOL! Well, that was only a promise to *myself*. I'm always breaking those.
Richard Heathfield
> Richard Heathfield <dont...@address.co.uk.invalid> wrote in
> news:bn2ib3$afv$1...@titan.btinternet.com:
>
>> My question is this: Have you /considered/ other possibilities,
>> especially w.r.t. maintenance issues, or are you content with the goto
>> in this context? That is, have you explored this runlevel idea, /sans/
>> goto? If so, what were your findings?
>
> Using goto for this isn't the 'best' solution for most people, especially
> with regards to maintenance. There are three considerations when making
> code, or, like this is a code template:
>
> o ease of writing
> o ease of reading
> o ease of maintenance
>
> The goto approach is not the best at any of these, but it is easy to
> write, easy to read /and/ easy to maintain.
Thanks for explaining. BTW ISTR proposing something along these lines whilst
working for an airline company about 7 or 8 years ago, and it was laughed
out of court, because "every other statement will be a goto! You have /got/
to be kidding!" and, of course, I wasn't really all that keen on the idea
myself, so I didn't pursue it. Mind you, I hadn't thought of your runlevel
idea; it might just have sold the package to my team.
>
> It eventually grew out of the application of some of the techniques seen
> elsethread. It is fact exactly equivalent to the nested-if technique you
> posted. The big problem I have with that technique is that I find it
> difficult to keep track of which allocation code is associated with which
> cleanup code.
That's interesting, because I find it quite intuitive, and I suppose I just
assumed that other people do too.
> Summary: yes, I have considered other options, and the goto one is the
> one I like 'best'. :)
Or dislike least, perhaps?
Christopher Benson-Manica
> 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"!).
I'm curious, how do you resolve situations such as the one you presented
without using continue? It seems to me that the main alternative is to just
wrap everything in if{} else if{} ..., which seems substantially less clean
than the code above. Same thing goes for break - perhaps I rely too much on
its convenience, but to me it is very convenient indeed...
--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cyberspace.org | don't, I need to know. Flames welcome.
Micah Cowan
> On Wed, 22 Oct 2003 17:50:40 -0500, Ed Morton <mortonAVO...@lucent.com>
> wrote:
>
> >
> >
> >rihad wrote:
> ><snip>
> >> 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.
> >
>
> Not *exactly*. ((void *) 0) is a common way to represent 0 - the null pointer
> constant - behind the NULL macro.
No. 0 is *a* null pointer constant; (void*)0 is the *other* null
pointer constant. Reread 6.3.2.3#3.
--
Micah J. Cowan
mi...@cowan.name
The Real OS/2 Guy
> On Wed, 22 Oct 2003 11:09:48 +0000 (UTC), "The Real OS/2 Guy"
> <os2...@pc-rosenau.de> wrote:
>
> >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
> > (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...
>
> ... by writing code that emulates gotos. Of course you can get rid of gotos, by
> introducing a level of abstraction over gotos (like extra logic etc etc). I
> really don't understand you people, those who say that gotos are evil, but I'm
> trying real hard :)
No emulation of goto is needed when the design is clean and fine.
Startup to design the code in the manner
- describe what is to do. When it gets more complex bereak it up in
lower levels
- then when all what is defined make the HOW is it done.
make thinks simple, make it more simple if it is complicate. The
result will be significant less error and significant less debug time.
And a side effect is you would not write a single got - because its
not needed.
> >
> >Good design in programming level is:
> >
> >- a fuction exeeding substantial more than one screen page
> > is buggy per design.
>
> This is hilarious. Are we talking about 80x25 terminal emulation screen or
> 1600x1200 gvim in a maximized window? :)
The time where a screen was only 25 lines long is gone more than 10
years ago. At that time we had functions with about 2 screens in
length. With a screen resolution of 1280x1024 you get more than 80
lines and even with 1024x768 you gets more than 50 lines.
That rule is really old - it comes from the time whereas a tty was a
modern console and a line printer the standard print device. So 50
lines (is the net number of lines of a page of paper. So you had a
block of pages of paper to study until you got another time on a
terminal. It was easier to have a function on one side - and now
whereas you doesn't needs to print anything you have a screen that is
good enough to hold even such page in once.
> Seriously, if what a function does can be considered atomic, if there are no
> smaller useful subactions that could warrant making it out of that function into
> functions of their own, then it can get as long as it gets. It doesn't happen
> *usually*, but the function *may* get as long as it gets.
A fuction that contains multiple nedsted loops is surely not atomic.
It is quickly on a point whereas you does more work on too much
details at once. Breaking that up to what is done - and that in how
get its done makes it not only more readable but makes debugging that
more easy. E.g. quick step over instead of fiddling around with
breakpoints when you knows that this detail does as it should.
Write goto and you goes searching the continous point in the same
logic level - and the destination of a goto can be somewhere. Write a
return and you reads it as 'this level of work is done. Continue on
the next higher level. In debug you can even quick step over the
fucntion without seeing the detail.
Richard Heathfield
> Richard Heathfield <dont...@address.co.uk.invalid> spoke thus:
>
>> 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"!).
>
> I'm curious, how do you resolve situations such as the one you presented
> without using continue?
if(line[0] != '#') /* comment - skip it */
{
rc = ProcessLine(line);
}
> It seems to me that the main alternative is to
> just wrap everything in if{} else if{} ..., which seems substantially less
> clean
> than the code above.
My alternative looks pretty clean to me. :-)
> Same thing goes for break - perhaps I rely too much
> on its convenience, but to me it is very convenient indeed...
I don't dispute the convenience of break and continue to the writer of the
code. It is their inconvenience to the /reader/ that gives me pause.
CBFalconer
>
>
> 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.
What you really want is the variable visibility and restrictions
found by using nested procedures, as in Pascal. The best
approximation of this you can get in C involves using separate
files and static file scope variables. This has the disadvantage
that things that should really be automatic aren't, so that their
memory is not released when not needed, and the simultaneous
advantage that routines and variables can be global to multiple
functions.
At the same time such usage makes the routines non-reentrant and
non-thread-safe, due to the lack of automatic storage for the
variables.
--
Chuck F (cbfal...@yahoo.com) (cbfal...@worldnet.att.net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home.att.net> USE worldnet address!
Christian Bau
"The Real OS/2 Guy" <os2...@pc-rosenau.de> wrote:
> 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.
Does this need any comment? I don't think so.
Sheldon Simms
I hesitate to get too personal, but I just have to ask.
Were you kicked out of Germany because of your pro-goto opinions
Christian?
Ian Woods
> Ian Woods wrote:
>
>> Richard Heathfield <dont...@address.co.uk.invalid> wrote in
>> news:bn2ib3$afv$1...@titan.btinternet.com:
>>
<snip>
>> It eventually grew out of the application of some of the techniques
>> seen elsethread. It is fact exactly equivalent to the nested-if
>> technique you posted. The big problem I have with that technique is
>> that I find it difficult to keep track of which allocation code is
>> associated with which cleanup code.
>
> That's interesting, because I find it quite intuitive, and I suppose I
> just assumed that other people do too.
I can easilly handle it for the first few ifs. The problem is that after
4 or so, I start to lose track and am quite likely to put the right code
at the wrong indentation level when update the code.
It was when I was allocating a dozen or so non-trivial 'things' that I
started exploring new ways of doing this. What I came up with originally
is 'less offensive' in that it doesn't use goto's, but it takes more
effort to write and only obviously applies to OO style objects in C:
struct Foo {
int rl;
ObjType1 * one;
ObjType2 * two;
...
ObjType12 * twelve;
};
struct Foo * FooAlloc() {
struct Foo * f;
int status;
status=FooAllocInner(&f);
if (!status) {
FooDealloc(f);
f=NULL;
}
return f;
}
int FooAllocInner(struct Foo ** f) {
*f=NULL;
*f=malloc(sizeof **f);
if (!*f) return 0;
*f->rl=0;
*f->one=OneAlloc();
if (!*f->one) return 0;
*f->rl++;
...
*f->one=TwelveAlloc();
if (!*f->twelve) return 0;
*f->rl++;
return 1;
}
void FooDealloc(struct Foo * f) {
if (!f) return;
switch(f->rl) {
case 12: TwelveDealloc(f->twelve);
...
case 1: OneDealloc(f->one);
case 0: free(f);
}
}
The goto based version isn't as neat as this, but it is much more
applicable.
>> Summary: yes, I have considered other options, and the goto one is
>> the one I like 'best'. :)
>
> Or dislike least, perhaps?
It's the best approach I've found so far out of all the ones I've tried.
I don't have any strong dislikes about it, except for the 'verbiage' of
repeated "if (!x) goto_err; else rl++;" lines. Judicious use of a macro
eliminates that almost painlessly, so I don't have any real complaints...
Well, apart from the use of goto which I occasionally get frowned at for.
:)
Out of all this discussion though, I've actually worked out a good rule
for when to use goto! Use goto when you need control-flow your
programming language doesn't natively support.
That's what the 'runlevel' example of mine does, and may be why it isn't
completely offensive to everyone (including me!).
Ian Woods
The Real OS/2 Guy
wrote:
> The Real OS/2 Guy wrote:
> >
> ... snip ...
> >
> > 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.
>
> What you really want is the variable visibility and restrictions
> found by using nested procedures, as in Pascal. The best
> approximation of this you can get in C involves using separate
> files and static file scope variables. This has the disadvantage
> that things that should really be automatic aren't, so that their
> memory is not released when not needed, and the simultaneous
> advantage that routines and variables can be global to multiple
> functions.
Won't help really. The project was an OS for a special kind of
processor family.
About 4,000,000 lines of code in about 400 modules. And a team of 40
developers working together.
Enty gate: user API calls comes here in
IRQ gate : IRQ comes her in and gets dispatched to its real sourse
This was done because an IRQ was commony coming from an
multiple cascaded hardware interface (256 * 256 * n)
devices on one.
So each of the entry points had either to call some functions with 2
to 200 parameters or to put anything on static places and make the
call parameterless. Parameterless calls are cheap in runtime, up tu 4
parameters are normal and more parameters were really expensive in a
call. So saving parameters was the main point to get runtime save
functionality.
There were 4 differen address spaces to handle:
- user addressroom data
- user addressroom code
- OS internal addressroom data
- OS internal addressroom code
Each addressroom was limited to 64 KB. Swappig addressroom was really
expensive on CPU, so the call cate had to collect the parameters from
user addressroom, store them inside the OS addressroom (including data
coming in as pointers, but excluding disk and other media buffers) to
save any driver and fuction to catch them theyrself.
But as that there were some experienced programmers who were crying
that global data is evil. Yes global data IS evil - but inside an OS
performance is more relevant than avoiding globals. Breaking code in
deep levels of functions, breaking it up in different translation
units is standard - even inside an OS. Has nothing to do with pascal -
except that pascal is unable to translate multiple soursec seapartele
and bind statically with assembler modules.
> At the same time such usage makes the routines non-reentrant and
> non-thread-safe, due to the lack of automatic storage for the
> variables.
Threadsave is impossible when you have exactly one interrupt level you
runs in and this is the highest below NMI and hardware IRQ level. So a
call from a user runs exclusive on the CPU until it comes back to the
user gate that gives the CPU back to a user function - except an NMI
or hardware IRQ occures. So saving CPU time is most critical in an
realtime OS to let all CPU ever possible the user.
Dan Pop
What about all the other null pointer constants, like 0L, (1 - 1L),
(void *)(sizeof(int) - sizeof(unsigned)) and so on? ;-)
Christopher Benson-Manica
> if(line[0] != '#') /* comment - skip it */
> {
> rc = ProcessLine(line);
> }
> My alternative looks pretty clean to me. :-)
Squeaky, even! I take it back ;)
> I don't dispute the convenience of break and continue to the writer of the
> code. It is their inconvenience to the /reader/ that gives me pause.
True. Although unlike goto, there's a limit to how convoluted they can get...
rihad
Ok here comes my first lesson on proper usage of English "a" and "the" :)
I still prefer using 0 over NULL for the snipped reasons.
Dan Pop
>I still prefer using 0 over NULL for the snipped reasons.
No matter what the snipped reasons are, it's a bad idea: NULL makes the
code more readable, because it emphasises the pointer context.
Micah Cowan
Yes, I know I was oversimplifying: it'd have been more accurate
to say "groups of null pointer constants", perhaps...
Damn, guess I shoulda known someone'd call me on that. :-)
--
Micah Cowan
mi...@cowan.name
Dan Pop
>Dan...@cern.ch (Dan Pop) writes:
>
>> In <m33cdj4...@localhost.localdomain> Micah Cowan <mi...@cowan.name> writes:
>>
>> >rihad <ri...@mail.ru> writes:
>> >
>> >> On Wed, 22 Oct 2003 17:50:40 -0500, Ed Morton <mortonAVO...@lucent.com>
>> >> wrote:
>> >>
>> >> >
>> >> >
>> >> >rihad wrote:
>> >> ><snip>
>> >> >> 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.
>> >> >
>> >>
>> >> Not *exactly*. ((void *) 0) is a common way to represent 0 - the null pointer
>> >> constant - behind the NULL macro.
>> >
>> >No. 0 is *a* null pointer constant; (void*)0 is the *other* null
>> >pointer constant. Reread 6.3.2.3#3. ^^^^^^^^^^^
>>
>> What about all the other null pointer constants, like 0L, (1 - 1L),
>> (void *)(sizeof(int) - sizeof(unsigned)) and so on? ;-)
>
>Yes, I know I was oversimplifying: it'd have been more accurate
>to say "groups of null pointer constants", perhaps...
Much simpler: write "*another*" instead of "the *other*".
Micah Cowan
> In <m37k2ta...@localhost.localdomain> Micah Cowan <mi...@cowan.name> writes:
>
> >Dan...@cern.ch (Dan Pop) writes:
> >
> >> In <m33cdj4...@localhost.localdomain> Micah Cowan <mi...@cowan.name> writes:
> >>
> >> >No. 0 is *a* null pointer constant; (void*)0 is the *other* null
> >> >pointer constant. Reread 6.3.2.3#3. ^^^^^^^^^^^
> >>
> >> What about all the other null pointer constants, like 0L, (1 - 1L),
> >> (void *)(sizeof(int) - sizeof(unsigned)) and so on? ;-)
> >
> >Yes, I know I was oversimplifying: it'd have been more accurate
> >to say "groups of null pointer constants", perhaps...
>
> Much simpler: write "*another*" instead of "the *other*".
Quite right: but I had meant to point out the two categories
under which a null pointer constant can fall (according to the
Standard), and was simply damned lazy about it. :-)
Sudheer Reddy Vakati
>That's interesting, because I find it quite intuitive, and I suppose I just
>assumed that other people do too.
>
>
>
understandability of the runlevel code given by Ian, i was able to
understand it at first go. If i am able to understand it at first go i
dont see why i shouldn't use the idea presented by Ian even though it
contains a goto.
VSR