clean exit - suggestion - C version

[lallous]

Hello,

This question was asked in comp.lang.c++ and the answers involved the use of
objects whose destructors are automatically called when getting out of
scope, however I was expecting suggestions that doesn't involve the use of
objects. So here is the question again.

I have a function like:

void fnc() {
char *mem1, *mem2, *mem3, *mem4;
FILE *fp1;
// lots of code...
// mem1, 2, 3, 4 got allocated
// lots of code and condition checks
if (condition_failed)
{
// blah blah
// free mem1, mem2, mem3, mem4
return;
}

if (condition2_failed)
{
// blah blah
// free mem1, mem2, ...
fclose(fp1);
return;
}

// here the end of routine (clean exit code):
// free mem1, mem2, mem3, mem4
}

Usually, I would use compiler specific solution by putting most of the code
in __try() and the clean exit code in _finally() block then to reach the
clean exit code I would invoke __leave.

Or use lables and then goto clean_exit

Any better way, other than goto or compiler specific solution?

 

[Peter Pichler]

Hello,
...
I have a function like:

void fnc() {
char *mem1, *mem2, *mem3, *mem4;
FILE *fp1;
// lots of code...
// mem1, 2, 3, 4 got allocated
// lots of code and condition checks
if (condition_failed)
{
// blah blah
// free mem1, mem2, mem3, mem4
return;
}

if (condition2_failed)
{
// blah blah
// free mem1, mem2, ...
fclose(fp1);
return;
}

// here the end of routine (clean exit code):
// free mem1, mem2, mem3, mem4
}

Usually, I would use compiler specific solution by putting most of the code
in __try() and the clean exit code in _finally() block then to reach the
clean exit code I would invoke __leave.

Or use lables and then goto clean_exit

Any better way, other than goto or compiler specific solution?

A never ending story... You will get that many replies as many posters there
are on Usenet ;-)

My prefered solution is:

void fnc (void)
{
char * const mem1 = something_that_allocates_mem1();
if (mem1)
{
char * const mem2 = something_that_allocates_mem2();
if (mem2)
{
/* ...etc. */
free(mem2);
}
free(mem1);
}
}

This has, IMO, two main advantages:
1. It's much clearer, variables are only declared where and when they are
actually used.
2. Memory is freed in the reverse order of allocation, which is usually
considered a good practice (though it should make no difference).

On the other hand, it may get a bit cumbersome if you have too many
variables to clear.

But as I said, it is all merely a matter of style. Use what you think is the
best (or your company coding guidelines dictate you).

Peter

 

[CBFalconer]

.... snip ...

A never ending story... You will get that many replies as many
posters there are on Usenet ;-)

My prefered solution is:

void fnc (void)
{
char * const mem1 = something_that_allocates_mem1();
if (mem1)
{
char * const mem2 = something_that_allocates_mem2();
if (mem2)
{
/* ...etc. */
free(mem2);
}
free(mem1);
}
}

and my version is:

void fnc(void)
{
char *cp1;
char *cp2;
char *cp3;
/* as needed */

if (!(cp1 = malloc(CP1SIZE))) goto cp1fail;
else if (!(cp2 = malloc(CP2SIZE))) goto cp2fail;
else if (!(cp3 = malloc(CP3SIZE))) goto cp3fail;
else {
/* they all worked - use em */
}
cp3fail:
free(cp3);
cp2fail:
free(cp2);
cp1fail:
free(cp1);
}

which is easily extended/compressed to the things needed. For
cleanup with free we can rework this to eliminate the gotos and
simply use NULL initializations for the cpN, but the above
organization will work with more complex scenarios.

 

[lallous]

and my version is:

void fnc(void)
{
char *cp1;
char *cp2;
char *cp3;
/* as needed */

if (!(cp1 = malloc(CP1SIZE))) goto cp1fail;
else if (!(cp2 = malloc(CP2SIZE))) goto cp2fail;
else if (!(cp3 = malloc(CP3SIZE))) goto cp3fail;
else {
/* they all worked - use em */
}
cp3fail:
free(cp3);
cp2fail:
free(cp2);
cp1fail:
free(cp1);
}

which is easily extended/compressed to the things needed. For
cleanup with free we can rework this to eliminate the gotos and
simply use NULL initializations for the cpN, but the above
organization will work with more complex scenarios.

My question was:
1)I have allocated lots of resources (memory, files, ...)
2)for some conditions other than allocation failures the code will
have to exit and clean up, but instead of cleaning up everytime and
repeating the clean up code, the clean up code must be written once
and called many times by some technique such as 'goto'

// here resources are allocated
// here some condition
if (failed)
{
// cleanup code
}
// some code
// some condition
if (failed2)
{
// same cleanup code as above.....
}

 

[Nils Petter Vaskinn]

My question was:
1)I have allocated lots of resources (memory, files, ...)
2)for some conditions other than allocation failures the code will
have to exit and clean up, but instead of cleaning up everytime and
repeating the clean up code, the clean up code must be written once
and called many times by some technique such as 'goto'

Initialize all pointers to NULL.
Use true false value to indicate successful allocation of resources where
the handle itself (such as a file handle) can't be used to check if the
resource should be freed. If you detect an error use a goto.

This works for the simple cases, but I'd bet someone could come up with an
example where the method would fail.

int fn() {

char *mem1 = NULL;
char *mem2 = NULL;
char *mem3 = NULL;
char *mem4 = NULL;
FILE *fp1 = NULL;
int some_resource_flag = 0;

int result;

/* allocate resources, goto cleanup on failure */

cleanup:
if (some_resource_flag) free_some_resource();
if (fp1) fclose(fp1);
free(mem4);
free(mem3);
free(mem2);
free(mem1);

return result;

}

 

[Kamilche]

I like all those solutions posted above, and have used them all
myself. What's not covered, however, is leaving the function the way
it is! :-D That's my current 'fave method'. Early programming exits
are supposedly a 'bad thing', but they occasionally have advantages
over the other methods when:

1. The cleanup code to be executed varies from exit to exit.
2. The remaining code is large, and doing it in one of the above
methods results in heavily indented code.

Using the other two methods, it's impossible to be consistent - it's
appropriate only occasionally. The 'early exit' method is useful more
frequently, but sometimes results in the redundant line or two.

In general, if early exits will give me what I want with no duplicate
code, or only one line, I'll use it. If the cleanup code is
significant, I use one of the methods outlined by the others. I prefer
the clean reading style you get with early exits, so I use it most
frequently. This is a change from my C coding style as a beginner.

//----------------------------------
// Here's an example of its use:
//----------------------------------
if (!A)
return; // error condition 1
if (!B)
return; // error condition 2
if (!C)
return; // error condition 3.

// If the code got here, it passed all the tests.
dofunction(D);


//----------------------------------
// Without it, the code would look like:
//----------------------------------
if (A)
if (B)
if (C)
dofunction(D);
else
return;
else
return;
else
return;

In this trivial example, the 'else returns' are optional and can be
left off... but if they're supposed to do cleanup, you can see where
this indentation could get quite large. Also, with this method, it's
easy to lose track of where you are in the 'if' hierarchy, and
inadvertantly execute some logic you weren't supposed to. I haven't
had that problem since switching to the 'early exit' style of
programming.


--Kamilche

 

[Nils Petter Vaskinn]

I like all those solutions posted above, and have used them all myself.
What's not covered, however, is leaving the function the way it is! :-D
That's my current 'fave method'. Early programming exits are supposedly
a 'bad thing', but they occasionally have advantages over the other
methods when:

1. The cleanup code to be executed varies from exit to exit. 2. The
remaining code is large, and doing it in one of the above methods
results in heavily indented code.

1. My suggestion to have a flag for each resource to indicate if it should
be freed or not will usually eliminate the need for different cleanups.
Since checking the flags takes care of the different cases

2. The goto method I described above shouldn't increase the indentation
level significantly unless you use a very bizarre style.

Using the other two methods, it's impossible to be consistent - it's
appropriate only occasionally. The 'early exit' method is useful more
frequently, but sometimes results in the redundant line or two.

In general, if early exits will give me what I want with no duplicate
code, or only one line, I'll use it. If the cleanup code is significant,
I use one of the methods outlined by the others. I prefer the clean
reading style you get with early exits, so I use it most frequently.
This is a change from my C coding style as a beginner.

[snip example]

Your early exit exaple doesn't really fit in this discussion since there
is no cleanup.

But "return;" and "return something;" can easily be replaced with

/* result = something if there is a return from the function */
goto cleanup;

If there was any cleanup to be done, so the body of the code would
essentially be the same as with early exit. But you avoid having the
cleanup code at every exit point.

What it all boils down to is trying to make the code easy to understand
and maintain. Your example is, so it's perfectly ok code (IMO) but I would
try to avoid early exit in some other cases (like in one single case
hidden deep within nested ifs and loops where a quick glance at the
function could lead one to believe it has only a single exit point.

 

[CBFalconer]

.... snip ...

My question was:
1)I have allocated lots of resources (memory, files, ...)
2)for some conditions other than allocation failures the code will
have to exit and clean up, but instead of cleaning up everytime and
repeating the clean up code, the clean up code must be written once
and called many times by some technique such as 'goto'

which is just what my pattern does. malloc/free are just one
examples of routines that setup and cleanup.

if (setupfails(params1)) goto clean1;
else if (setupfails(params2)) goto clean2;
else {
/* success */
}
clean2: cleanup(params1);
clean1: cleanup(params2);

 

[Martijn]

void fnc() {

char *mem1, *mem2, *mem3, *mem4;
FILE *fp1;
// lots of code...
// mem1, 2, 3, 4 got allocated
// lots of code and condition checks
if (condition_failed)
{
// blah blah
// free mem1, mem2, mem3, mem4
return;
}

if (condition2_failed)
{
// blah blah
// free mem1, mem2, ...
fclose(fp1);
return;
}

// here the end of routine (clean exit code):
// free mem1, mem2, mem3, mem4
}

I have a method I use only when I feel it really clarifies the situation,
and I always comment it (with a few words) when I'm doing so. It assumes
that all pointers (or objects in your case) are set to such a state that
cleaning them when left untouched isn't a problem or that it is obvious when
they don't need to be cleaned.

char *mem1 = NULL, *mem2 = NULL, *mem3 = NULL;
/* or you could do this:
char *mem1, *mem2, *mem3;
mem1 = mem2 = mem3 = NULL;
*/

/* try-like exception construct */
do
{
mem1 = malloc(MEM1SIZE);
if ( !mem1 )
break;

/* do something here */

mem2 = malloc(MEM1SIZE);
if ( !mem2 )
break;

/* do something else here */

mem1 = malloc(MEM1SIZE);
if ( !mem3 )
break;

/* do even more here */
}
while ( 0 );

if ( mem1 ) free(mem1);
if ( mem2 ) free(mem2);
if ( mem3 ) free(mem3);

return;

The do { } while ( 0 ) will always be executed once. You could put a return
code in there if you want to as well.

I think it is a decent solutions, much like the goto solution, but the
reason I prefer this over the goto approach is because it is very obvious
what block of code is being "tried" and it bears a little more resemblance
to a real try/catch construct.

Hope this helps,

Martijn Haak
http://www.sereneconcepts.nl

 

[Kamilche]

What it all boils down to is trying to make the code easy to understand
and maintain. Your example is, so it's perfectly ok code (IMO) but I would
try to avoid early exit in some other cases (like in one single case
hidden deep within nested ifs and loops where a quick glance at the
function could lead one to believe it has only a single exit point.

Yeah, consistency with the rest of the code is important! I use early
exits very frequently in a common set of modules, so when you're
reading it, exceptions are pulled out and 'legal code' is in a
straight line down the page. But if the code didn't use early exits so
heavily, I would hesitate to include it - it's best to follow the
convention of the existing codebase.

I read about a study they did on experienced programmers versus newbie
programmers. As long as the code did 'the expected thing' and followed
a convention of some sort, the experienced programmers were far more
productive. But if the code did it one way in this module, another way
in another module, and was relatively a mishmash, they were just as
unproductive as the newbie programmers.

Far from being the 'hobgoblin of little minds', consistency extends a
programmer's brainpower. Use it in the style most suited to YOUR
brain.

--Kamilche

 

[lallous]

I have a method I use only when I feel it really clarifies the situation,
and I always comment it (with a few words) when I'm doing so. It assumes
that all pointers (or objects in your case) are set to such a state that
cleaning them when left untouched isn't a problem or that it is obvious when
they don't need to be cleaned.

char *mem1 = NULL, *mem2 = NULL, *mem3 = NULL;
/* or you could do this:
char *mem1, *mem2, *mem3;
mem1 = mem2 = mem3 = NULL;
*/

/* try-like exception construct */
do
{
mem1 = malloc(MEM1SIZE);
if ( !mem1 )
break;

/* do something here */

mem2 = malloc(MEM1SIZE);
if ( !mem2 )
break;

/* do something else here */

mem1 = malloc(MEM1SIZE);
if ( !mem3 )
break;

/* do even more here */
}
while ( 0 );

if ( mem1 ) free(mem1);
if ( mem2 ) free(mem2);
if ( mem3 ) free(mem3);

return;

The do { } while ( 0 ) will always be executed once. You could put a return
code in there if you want to as well.

I think it is a decent solutions, much like the goto solution, but the
reason I prefer this over the goto approach is because it is very obvious
what block of code is being "tried" and it bears a little more resemblance
to a real try/catch construct.

Hope this helps,

Martijn Haak
http://www.sereneconcepts.nl

Martijn, I like your do/while approach, it really looks like the
try/leave/finally block to me!

Thanks to all who posted their suggestions they were also useful in a way or
another.