The advantage there is that you can cleanup code even if you have a multiple nested scope (break doesn't work for that), and you assert/log at the site of the error in debug builds. The disadvantage is that everyone jumps down your throat because you used goto (it being "evil" and all that". The way I rationalize it is that I'm hardly using it for complex flow control, and it significantly increases the readability of the code.
The "wrap it inside a scope and use break" idea is good, think I'll use that from now, though I'm probably gonna use "do { ... } while(0);" instead of a for(;;) with return.
"even if this code was "good" and had a different error return value for each type of error, I hate that shit, because it doesn't help me debug and get a breakpoint right at the point where the error is happening" A nice way to do this is just:
then use "return Error(blah);" instead of "return blah;". Then you can put an assert into Error() (either always fault or fault on specific errors) and you can easily grep for paths that return error codes.
Though I should be clear that really the preferred way is C++ wrappers and this is just sort of the least-bad way.
It's still just too easy to slip in a return somewhere and fail to clean up, or to write the wrong cleanup code. There's absolutely no reason why objects should not be self-cleaning.
If for some weird reason you can't do that, you can also get the same effect using an on-destruct callback.
Why is the break example better than gotos? Using break has all the usual problems with breaks if you have any nesting. Adding a nesting or being at all careless with cut'n'paste will screw you. I now regard break as a bug waiting to happen for any loop longer than 20 lines.
"Just because break works with C++ and goto doesn't." Where's that coming from, btw?
It gets tricky if you just jump into the middle of some scope, but if you put a label right after the end of a scope you're currently nested in those problems don't occur (the main problem is visibility of symbols that will be declared in the current block but haven't been declared yet - a non-issue if you jump into a scope where those symbols aren't visible anyway).
As far as I can tell, "emulating" breaks (or multi-level breaks) with gotos is always safe. Any specific case where that doesn't work?
Yeah, it's fine as long as you just emulate a break, but what people actually do with gotos is something like
ret = false;
if ( ! (A = makeA) ) goto fail;
if ( ! (B = makeB) ) goto fail;
ret = true;
fail:
cleanup(A); cleanup(B);
return ret;
the nasty thing, why I think goto should be avoided, is that if you happen to write a function like this that doesn't use any classes, then it will work just fine. Now you have created a function that I cannot use C++ in without rewriting how you do cleanup.
So what I'm trying to say is that "what people mean when they say goto-style error handling doesn't work with C++".
If you write it in the "break style", then sure you can use goto instead of break if you prefer.
The only good way to solve this is by using RAII. No other method, goto, break or whatever, is going to be as complete.
If there is an exception thrown in any function called from within the function then the resources will not be cleaned up correctly (unless you plan on adding try/catch everywhere).
If this were my code, I would write a sharable ref counted class to wrap the file. No clean up code is needed then. And clean up, in the event of an exception, is automatically handled when the stack is cleaned up.
The following code is of the top of my head, doesn't test for valid file pointer etc. but otherwise, is the best way to handle resource management in C++.
Michael, I believe I said that already. Several times.
February 27, 2011 at 7:32 PM
I hate code that does error cleanup in multiple places, eg :
FILE * fp = fopen(fileName,"wb");
if ( ! stuff1() )
{
fclose(fp);
return false;
}
if ( ! stuff2() )
{
fclose(fp);
return false;
}
// ok!
return true;
the error cleanup has been duplicated and this leads to bugs.
In the olden days we fixed this by putting the error return at the very end (after the return true) and using
a goto to get there. But gotos don't play nice with C++ and are just generally deprecated. (don't get me
started on setjmp, WTF is libpng thinking using that archaic error handling system? just because you think
it's okay doesn't mean your users do)
Obviously the preferred way is to always use C++ classes that clean themselves up. In fact whenever someone
gives me code that doesn't clean itself up, I should just immediately make a wrapper class that cleans itself up.
I find myself getting annoyed and having bugs whenever I don't do this.
There is, however, a cleanup pattern that works just fine. This is well known, but I basically never ever see
anyone use this, which is a little odd. If you can't use C++ self-cleaners for some reason, the next best
alternative is using "break" in a scope that will only execute once.
For example :
rrbool rrSurface_SaveRRSFile(const rrSurface * surf, const char* fileName)
{
FILE * fp = fopen(fileName,"wb");
if ( ! fp )
return false;
for(;;)
{
rrSurface_RRS_Header header;
if ( ! rrSurface_FillHeader(surf,&header) )
break;
if ( ! rrSurface_WriteHeader(fp,&header) )
break;
if ( ! rrSurface_WriteData(fp,surf,&header) )
break;
// success :
fclose(fp);
return true;
}
// failure :
fclose(fp);
return false;
}
Really the break is just a simple form of goto that works with C++. When you have multiple things to cleanup obvious you have to
check each of them vs uninitialized.
(BTW this example is not ideal because it doesn't give you any info about the failure. Generally I think all code
should either assert or log about errors immediately at the site where the error is detected, not pass error codes
up the chain. eg. even if this code was "good" and had a different error return value for each type of error, I
hate that shit, because it doesn't help me debug and get a breakpoint right at the point where the error is happening.)
ADDENDUM :
Another common style of error cleanup is the "deep nest with partial cleanup in each scope". Something like this :
bool success = false;
if ( A = thing1() )
{
if ( B = thing2() )
{
if ( C = thing3() )
{
success = true;
cleanup C;
}
cleanup B;
}
cleanup A;
}
I really hate this style. While it doesn't suffer from duplication of the cleanups, it does break them into pieces.
But worst, it makes the linear code flow very unclear and introduces a deep branching structure that's totally unnecessary. Good code should be a
linear sequence of imperatives as much as possible. (eg. do X, now do Y, now do Z).
I think this must have been an
approved Microsoft style at some point because you see it a lot in MSDN samples; often the success code path winds up
indented so far to the right that it's off the page!
"02-23-11 - Some little coder things - Error cleanup with break"
11 Comments -
I use a slightly different idiom, but only for error handling.
#define breakAndGotoLabel(LBL) \
do { \
assert(0); \
goto LABEL; \
} while (0)
void func()
{
if (someError)
breakAndGotoLabel(FAIL)
// success
return true;
FAIL:
cleanup();
return false;
}
The advantage there is that you can cleanup code even if you have a multiple nested scope (break doesn't work for that), and you assert/log at the site of the error in debug builds. The disadvantage is that everyone jumps down your throat because you used goto (it being "evil" and all that". The way I rationalize it is that I'm hardly using it for complex flow control, and it significantly increases the readability of the code.
February 23, 2011 at 7:05 PM
The "wrap it inside a scope and use break" idea is good, think I'll use that from now, though I'm probably gonna use "do { ... } while(0);" instead of a for(;;) with return.
"even if this code was "good" and had a different error return value for each type of error, I hate that shit, because it doesn't help me debug and get a breakpoint right at the point where the error is happening"
A nice way to do this is just:
"static inline ErrorCode Error(ErrorCode x) { return x; }"
then use "return Error(blah);" instead of "return blah;". Then you can put an assert into Error() (either always fault or fault on specific errors) and you can easily grep for paths that return error codes.
February 23, 2011 at 8:18 PM
You've still got your cleanup code in two places: success and failure. A simpler solution:
rrbool rrSurface_SaveRRSFile(const rrSurface * surf, const char* fileName)
{
FILE * fp = fopen(fileName,"wb");
if ( ! fp )
return false;
rrbool result = false; // we failed unless we succeed
do
{
rrSurface_RRS_Header header;
if ( ! rrSurface_FillHeader(surf,&header) )
break;
if ( ! rrSurface_WriteHeader(fp,&header) )
break;
if ( ! rrSurface_WriteData(fp,surf,&header) )
break;
// success :
result = true;
} while( 0 )
// finally :
fclose(fp);
return result;
}
February 24, 2011 at 8:39 AM
Yeah, ok.
Though I should be clear that really the preferred way is C++ wrappers and this is just sort of the least-bad way.
It's still just too easy to slip in a return somewhere and fail to clean up, or to write the wrong cleanup code. There's absolutely no reason why objects should not be self-cleaning.
If for some weird reason you can't do that, you can also get the same effect using an on-destruct callback.
In cblib I would do :
FILE * fp = fopen(..)
ON_DESTRUCT1(fclose,fp);
February 24, 2011 at 11:02 AM
Why is the break example better than gotos? Using break has all the usual problems with breaks if you have any nesting. Adding a nesting or being at all careless with cut'n'paste will screw you. I now regard break as a bug waiting to happen for any loop longer than 20 lines.
February 24, 2011 at 4:22 PM
" Why is the break example better than gotos? "
Just because break works with C++ and goto doesn't.
February 24, 2011 at 4:47 PM
"ON_DESTRUCT1(fclose,fp);"
Ugh, C++ lack of closures annoys me to no end!
I know, C++0x...
February 24, 2011 at 7:21 PM
"Just because break works with C++ and goto doesn't."
Where's that coming from, btw?
It gets tricky if you just jump into the middle of some scope, but if you put a label right after the end of a scope you're currently nested in those problems don't occur (the main problem is visibility of symbols that will be declared in the current block but haven't been declared yet - a non-issue if you jump into a scope where those symbols aren't visible anyway).
As far as I can tell, "emulating" breaks (or multi-level breaks) with gotos is always safe. Any specific case where that doesn't work?
February 25, 2011 at 11:38 AM
Yeah, it's fine as long as you just emulate a break, but what people actually do with gotos is something like
ret = false;
if ( ! (A = makeA) )
goto fail;
if ( ! (B = makeB) )
goto fail;
ret = true;
fail:
cleanup(A);
cleanup(B);
return ret;
the nasty thing, why I think goto should be avoided, is that if you happen to write a function like this that doesn't use any classes, then it will work just fine. Now you have created a function that I cannot use C++ in without rewriting how you do cleanup.
So what I'm trying to say is that "what people mean when they say goto-style error handling doesn't work with C++".
If you write it in the "break style", then sure you can use goto instead of break if you prefer.
February 25, 2011 at 6:48 PM
The only good way to solve this is by using RAII. No other method, goto, break or whatever, is going to be as complete.
If there is an exception thrown in any function called from within the function then the resources will not be cleaned up correctly (unless you plan on adding try/catch everywhere).
If this were my code, I would write a sharable ref counted class to wrap the file. No clean up code is needed then. And clean up, in the event of an exception, is automatically handled when the stack is cleaned up.
The following code is of the top of my head, doesn't test for valid file pointer etc. but otherwise, is the best way to handle resource management in C++.
class File
{
public:
File(FILE * fp):m_fp(fp){}
~File(){fclose(m_fp);}
protected:
private:
FILE * m_fp;
};
typedef boost::shared_ptr FilePtr;
FilePtr MyFunction(const char * pPath)
{
FilePtr spFile(new File(fopen(pPath)));
SomeFunctionWhichMightThrow();
return spFile;
}
February 27, 2011 at 4:43 PM
Michael, I believe I said that already. Several times.
February 27, 2011 at 7:32 PM