macOS goto: bailout a good strategy for cleanup in C?

[GorillaPaws]

I'm reading the recently released "Advanced Mac OS X Programming: The Big Nerd Ranch Guide" by Mark Dalrymple and have reached a section about using goto as an error handling technique. To quote the book:

Unfortunately, too many in the programming industry have been trained to have a knee-jerk reaction to goto, when in many cases it can lead to much more readable cleanup than the alternatives.

He then provides the following pseudocode (may contain typing errors because I can't compile it)

int someFunction( void )
{
	result = failure;
	blah = allocate_some_memory();
	if( do_something( blah ) == failure )
	{
		goto bailout;
	}
	
	ack = open_a_file();
	if( process_file( blah, ack ) == failure )
	{
		goto bailout;
	}
	
	hoover = do_something_else();
	if( have_fun( blah, hoover ) == failure )
	{
		goto bailout;
	}
	//	we survived!
	result = success;

bailout:
	if( blah ) free_the_memory( blah );
	if( ack ) close_the_file( ack );
	if( hoover ) clean_this_up( hoover );

	return result;
}

I'm wondering how others feel about this approach.

 

[subsonix]

I think it's a reasonable approach, without it in this case you would need to check if previous resources had been allocated if you fail in the second or last section. This approach will lead to a higher degree of consistency in regards to bail and clean up across functions. One caveat is Unix functions that often return -1 on failure, so a special case is needed in the clean up section, rather than if(foo).

 

[itickings]

Using goto is like programming in C++; It can be very clean, efficient and readable when used correctly. It can also be ugly and completely unreadable.

I'd place the proposed pseudocode among the former, while many real-world uses of goto tend to belong to the latter.

 

[Sander]

This is the canonical example of a perfectly reasonable use of goto. The alternative (deeply nested scopes) is much less readable, in my humble opinion.

 

[lloyddean]

This is a relatively common way of handling errors in 'C'. I just thought it important to remember to set initial variable conditions in order to work properly.

int someFunction(void)
{
    result  = failure;

    blah    = 0;    // IMPORTANT!!!
    ack     = 0;    // IMPORTANT!!!
    hoover  = 0;    // IMPORTANT!!!

    blah = allocate_some_memory();
    if ( do_something(blah) == failure )        { goto bailout; }
    
    ack = open_a_file();
    if ( process_file(blah, ack) == failure )   { goto bailout; }
    
    hoover = do_something_else();
    if ( have_fun(blah, hoover) == failure )    { goto bailout; }

    result = success;

bailout:

    if ( blah )     { free_the_memory(blah); }
    if ( ack )      { close_the_file(ack);   }
    if ( hoover )   { clean_this_up(hoover); }

    return result;
}

 

[chown33]

I recommend a naming convention. One that makes sense to you.

For example, I use the label CLEANUP: (yes, upper-case). If there is a separate piece of code that needs to execute on failures, I label it FAILED: or FAILURE:. The FAILED code may or may not end by going to CLEANUP.

At one time I used label names like CATCH_IO_ERROR, CATCH_RANGE_ERROR, or CATCH_WhateverCausedThe_ERROR. The cleanup code was labeled FINALLY. The idea was to have a labeling convention similar to the structure of @try/@catch/@finally in Obj-C, or try/catch/finally in Java. I wasn't happy with how it worked in real code, so I went back to the simpler CLEANUP and FAILED labeling.

I also name the error-condition variables according to their logical sense. If a non-zero value indicates an error, the variable name starts with "err_". If a non-zero value indicates success, "ok_". Then the conditionals look like:

if ( err_thing ) { thing cleanup }
if ( err_other ) { other cleanup }
if ( ! ok_doodad ) { doodad cleanup }

The main reason for this is it clearifies that you're using goto and labels for error handling. I think it matters more that you have a clear naming convention, and less what that convention is, as long as it's clear and concise.

 

[foidulus]

I always just write a new function if I need to repeat the same code multiple times like that, technically there is an almost infinitesimal increase in overhead due to pushing stuff on the stack, but if you are using it for error conditions than it shouldn't matter. That way you can also potentially re-use the cleanup code elsewhere if needed etc.

However as long as it's done carefully both ways are valid and readable.

 

[GorillaPaws]

I always just write a new function if I need to repeat the same code multiple times like that...

This is my first instinct, although you're potentially left with passing in a lot of arguments which is also ugly. In Robert Martin's "Clean Code" he mentions that the fewer parameters a function/method takes, the better.

It's interesting how many of you guys immediately gave this approach the thumbs up. I was worried that this was a really uncommon/unusual technique (based on the author's statement), but having so many responses from programmers I respect gives me a lot of confidence in this approach.

I especially like @chown33's suggestion of establishing a clear naming scheme. Naming conventions are particularly critical for me, because I'm dyslexic and they help anchor the pieces down instead of trying to remember too many pieces simultaneously.

It seems like this approach is a bit like using an Objective-C Block since it has the same effect of "capturing the parent's scope" eliminating the need to call a function with tons of parameters.

Thanks to all who took the time to respond. I appreciate your experience and feedback.

 

[firewood]

goto statements got a bad rap because they can be used to create a mess of unreadable spaghetti code.

Any construct that is used in a cleanly written, well though out, and commonly used program flow structure is pretty much identical to a good structured construct. This is true even if that construct has a goto statement right in the middle, rather than a pile of if-else-brackets. Perhaps even truer.

And you can also write an obfuscated mess without using a single goto. Doesn't make that code better.

 

[Sydde]

Personally, I really do like nested structure and do not find that it hinders readability, but that again is a matter of taste. You can easily visualize the lifespan of a resource by its scoping (braces/indent level) and, to me at least, the code looks cleaner. There is one possibly significant difference between nesting and this scheme with gotos, as written.

With nesting, resources are unwound: cleaned up in the reverse order in which they were acquired. With the "bailout:" label in this snippet, resources are cleaned up in the same order that they were created. I believe in most cases, unwinding is the sensible approach, but there may be unusual situations where you would need finer control over the order of cleanup. In such situations, goto and labels would indeed be the most efficient technique.

 

[foidulus]

This is my first instinct, although you're potentially left with passing in a lot of arguments which is also ugly. In Robert Martin's "Clean Code" he mentions that the fewer parameters a function/method takes, the better.

It's interesting how many of you guys immediately gave this approach the thumbs up. I was worried that this was a really uncommon/unusual technique (based on the author's statement), but having so many responses from programmers I respect gives me a lot of confidence in this approach.

I especially like @chown33's suggestion of establishing a clear naming scheme. Naming conventions are particularly critical for me, because I'm dyslexic and they help anchor the pieces down instead of trying to remember too many pieces simultaneously.

It seems like this approach is a bit like using an Objective-C Block since it has the same effect of "capturing the parent's scope" eliminating the need to call a function with tons of parameters.

Thanks to all who took the time to respond. I appreciate your experience and feedback.

If all you are passing is pointers then it basically makes no difference. Worst case you could always inline.

 

[MasConejos]

Just as an alternative error handling approach, I make use of setjmp.h and some macros to implement c++ style try/catch/finally blocks in standard c. Setjmp() saves the execution state (stack pointer, program counter, registers, etc) at the calling point, and longjmp() restores a given save point.

printf("Entering TRY/CATCH\n);
XTRY
{
	case XCODE:		//this gets executed first in try block
		printf("Executing code section\n");	
		TestFunction();
		break;
	case SOME_ERROR:	//explicit error handler
		printf("Caught SOME_ERROR\n");
		break;
	default:		//catches all other errors
		printf("Caught generic error: %d\n", ex.ErrorCode);
		break;
}	
XEND
printf("Exited TRY/CATCH\n");

---------------------

void TestFunction()
{
	RaiseException(SOME_ERROR);
}

Expanding the macros leaves:

printf("Entering TRY/CATCH\n");
while(1) 
{ 
	XData ex; 
	LinkException(&ex); 
	switch(ex.ErrorCode = setjmp(ex.Context)) 
	{
		case XCODE: 	//CODE
			printf("Executing code section\n");	
			TestFunction();
			break;
			
		case SOME_ERROR: //EXPLICIT HANDLER
			printf("Caught SOME_ERROR\n");
			break;
			
		default:	//ALL OTHER EXCEPTIONS
			printf("Caught generic error: %d\n", ex.ErrorCode);
			break;
	}
	UnlinkException(&ex); 
	break; 
}	
printf("Exited TRY/CATCH\n");

LinkException() and UnlinkException() add 'ex' to a global linked list of exceptions. RaiseException() sets ex.ErrorCode and calls longjmp(). I have another variation of this that allows for a FINALLY section that gets called at the end, regardless of whether errors have been raised or not.

The nice thing about this implementation is that these try/catch blocks can be nested and that everything they declare is on the stack, so there is no memory management needed.

I derived all of this on an article I read on error handling in C.