Question on goto with try/catch
TVisitor
style:
void MyClass::MyFunction()
{
try
{
// code here
if(something bad happened)
goto label;
}
catch(...)
{
// exception handling here.
}
label:
// Run cleanup code - deallocate memory, etc.
}
Personally, I would rather throw an exception instead of the goto, let
the catch do it's thing, and then the regular cleanup code will
execute.
However, from a "does it work?" standpoint, what are the implications
of using a goto to jump out of a try? Does it leave some uncleaned up
stuff behind (i.e. on the stack), or does it work properly, however
inelegant it looks? Please realize I'm don't wish to start an
unstructured looking code holy war, I'm questioning operation so I can
decide whether or not I need to rewrite this *now* or put it on a to-
do list for a later date. I'm not a big fan of rewriting code that
works - however inelegant - if it's been around and doing it's job.
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Mathias Gaunard
> However, from a "does it work?" standpoint, what are the implications
> of using a goto to jump out of a try? Does it leave some uncleaned up
> stuff behind (i.e. on the stack), or does it work properly, however
> inelegant it looks?
It should work properly, yes.
Alf P. Steinbach
> A project I am working on has large amounts of code written in this
> style:
>
> void MyClass::MyFunction()
> {
> try
> {
> // code here
> if(something bad happened)
> goto label;
> }
> catch(...)
Don't catch by ellipsis unless you're rethrowing (and possibly picking up the
exception again, but anyway rethrowing) within the catch handler.
For example, consider a badly designed exception that is a pointer to a
dynamically allocated exception object, like in Microsoft's MFC.
You wouldn't want to leak that.
> {
> // exception handling here.
> }
>
> label:
> // Run cleanup code - deallocate memory, etc.
> }
Mixed curse levels.
Redundancy.
> Personally, I would rather throw an exception instead of the goto, let
> the catch do it's thing, and then the regular cleanup code will
> execute.
An alternative that's easier to get right is to use a scopeguard (it's a RAII
based thingy).
Check out Petru Marginean & Andrei Alexandrescu's ScopeGuard article in some old
DDJ issue, available on the net.
However, for your "deallocate memory", simply do not do that manually. Even a
scopeguard is too low level for "deallocate memory". Use standard containers and
perhaps smart pointers if needed, or garbage collection if that's your thing.
> However, from a "does it work?" standpoint, what are the implications
> of using a goto to jump out of a try? Does it leave some uncleaned up
> stuff behind (i.e. on the stack), or does it work properly, however
> inelegant it looks?
Works properly, but is as noted mixing curse levels and redundant.
> Please realize I'm don't wish to start an
> unstructured looking code holy war, I'm questioning operation so I can
> decide whether or not I need to rewrite this *now* or put it on a to-
> do list for a later date. I'm not a big fan of rewriting code that
> works - however inelegant - if it's been around and doing it's job.
If it works don't fix it.
Cheers & hth.,
- Alf
Balog Pal
> void MyClass::MyFunction()
> {
> try
> {
> // code here
> if(something bad happened)
> goto label;
> }
> catch(...)
> {
> // exception handling here.
> }
>
> label:
> // Run cleanup code - deallocate memory, etc.
> }
DOH. You want to combine the disadvantages dropping the sunny side?
> Personally, I would rather throw an exception instead of the goto, let
> the catch do it's thing, and then the regular cleanup code will
> execute.
Move cleanup to where it belongs: to destructors of proper handler objects.
Then no more goto, and you can return or throw at any place you like -- and
possibly can scrap the try/catch block too. If not this one, then 100 of
its cousins.
> However, from a "does it work?" standpoint, what are the implications
> of using a goto to jump out of a try?
TMK you can jump out of blocks at will. The target location may be a
problem: you can't jump over object initializations unless they are fully
scoped, but in that case the code will not compile. If compiles it shall
work as advertised. Still you shoould leave goto to C code where you
don't have better tools.
> Does it leave some uncleaned up
> stuff behind (i.e. on the stack), or does it work properly, however
> inelegant it looks?
Elegance is not the problem -- that lies in handicraft of cleanup, that WILL
be messed up for sure, if not today then tomorrow.
> Please realize I'm don't wish to start an
> unstructured looking code holy war, I'm questioning operation so I can
> decide whether or not I need to rewrite this *now* or put it on a to-
> do list for a later date.
Ah, looking to incvest in technical debt on steep return rates, do ya?
Experience shows you start adding the mess like that, you create too much
work so 'later' will be even later and slipping no never. While you have
snowball-then-avalanche like supply of incoming problems.
> I'm not a big fan of rewriting code that
> works - however inelegant - if it's been around and doing it's job.
Ah, if it works do not touch it. But your asking the question implies you
NEED to touch it, so it doesn't do what is intended. Better put delusion
aside and face the ugly truth.
Nick Hounsome
> A project I am working on has large amounts of code written in this
> style:
>
> void MyClass::MyFunction()
> {
> try
> {
> // code here
> if(something bad happened)
> goto label;
> }
> catch(...)
> {
> // exception handling here.
> }
>
> label:
> // Run cleanup code - deallocate memory, etc.
>
> }
>
> Personally, I would rather throw an exception instead of the goto, let
> the catch do it's thing, and then the regular cleanup code will
> execute.
If the "bad thing" really is an exception then by all means throw an
exception.
> However, from a "does it work?" standpoint, what are the implications
> of using a goto to jump out of a try?
No more and no less than leaving any other block by goto or even
return.
> Does it leave some uncleaned up
> stuff behind (i.e. on the stack), or does it work properly, however
> inelegant it looks?
As far as the goto goes it is fine.
Destructors for anything declared in the try block are called on exit
however and whereever you exit the block.
> Please realize I'm don't wish to start an
> unstructured looking code holy war, I'm questioning operation so I can
> decide whether or not I need to rewrite this *now* or put it on a to-
> do list for a later date. I'm not a big fan of rewriting code that
> works - however inelegant - if it's been around and doing it's job.
The one real problem with this code is catch(...).
Unless you know all the things ... could be (in which case why not
catch them explicitly) it cannot be right to catch(...)
Suppose std::bad_alloc was thrown for example - Are you sure that none
of your handling code uses memory allocation either directly or
indirectly?
Whenever I see catch(...) the correct solution is invariably RAII
(especially given your comment at the label) - not only is it more
robust it is easier to understand too. In fact many people would argue
that 99% of the time if RAII isn't the answer then it probably isn't
really an exception (e.g. failing to open a file).
Summary:
catch() is for handling specific exceptions.
RAII is for handling general cleanup.
catch(...) is only really sensible for diagnostic trickery.
Goran
> However, from a "does it work?" standpoint, what are the implications
> of using a goto to jump out of a try? Does it leave some uncleaned up
> stuff behind (i.e. on the stack), or does it work properly, however
> inelegant it looks?
That does work correctly. In general:
{
TYPE var(params);
goto end;
} // var::~Type is executed here.
end:
It does not matter whether a {} block is a "try" block or not, there
is no problem. It would have been horrible if goto broke automatic
object cleanup.
That said, authors did not know how to write C++. This is not
uncommon, especially for old code, or for stubborn C people. One
should be wary of writing C in C++. Personally, seeing unwarranted C
code in cpp files makes me cringe.
The way your snippet is written, it hints that code in catch block
relies on state changes that happened in try block, e.g.
resource r = null_init();
try
{
// code code code
if (some_condition) r = get_resource();
// code code code
}
catch(something)
{
if (r.ready()) do_x();
else do_y();
}
label:
free_resource(r);
IMO, that's a very rare situation, and I bet that's not your case,
especially since you say that such code is abound.
The way this is written, that catch probably must not throw again,
otherwise resources that are supposed to be cleaned up after "label"
will leak. IOW, carefully with that catch.
Normally, one uses RAII to handle automatic cleanup when going out of
scope. Simplest possible example:
void MyClass::MyFunction()
{
TYPE* p = NULL;
try
{
p = new TYPE(params)
// code here
if(something bad happened)
goto label;
}
catch(...)
{
// exception handling here.
}
label:
delete p;
}
Using RAII, that becomes:
typedef std::auto_ptr<TYPE> PType;
void MyClass::MyFunction()
{
try
{
PType p(new TYPE(params));
// code here
if(something bad happened)
goto label;
} // TYPE* automatically deleted here through ~PType
catch(...)
{
// exception handling here.
}
}
If catch needs p (rare situation), this becomes:
void MyClass::MyFunction()
{
typedef std::auto_ptr<TYPE> PType;
PType p;
try
{
p = PType(new TYPE(params));
// code here
if(something bad happened)
goto label;
}
catch(...)
{
// exception handling here, using p if needed (watch out for NULL!).
}
} // TYPE* automatically deleted here through ~PType
Goran.
Richard Corden
> A project I am working on has large amounts of code written in this
> style:
>
> void MyClass::MyFunction()
> {
> try
> {
> // code here
> if(something bad happened)
> goto label;
> }
> catch(...)
> {
> // exception handling here.
> }
>
> label:
> // Run cleanup code - deallocate memory, etc.
> }
>
> Personally, I would rather throw an exception instead of the goto, let
> the catch do it's thing, and then the regular cleanup code will
> execute.
Yes its Ok.
What you cannot do is jump into a try block or a catch handler. But
jumping out is OK and should do the clean-up. Part of 15/2 has:
"...A goto, break, return, or continue statement can be used to transfer
control out of
a try block or handler. When this happens, each variable declared in the
try block will be destroyed in the
context that directly contains its declaration."
Regards,
Richard
Eric J. Holtman
news:he5rst$570$1...@news.eternal-september.org:
>
>
> Don't catch by ellipsis unless you're rethrowing (and possibly picking
> up the exception again, but anyway rethrowing) within the catch
> handler.
Shouldn't you catch(...) on a module boundary
(like at the main function of a thread), just to
ensure no exceptions cross into a possibly non-C++
area (like the OS).
Or would your answer be "You should know what
exceptions can get to that point, and catch
all of them explicitly"?
Nick Hounsome
> "Alf P. Steinbach" <al...@start.no> wrote innews:he5rst$570$1...@news.eternal-september.org:
>
>
>
> > Don't catch by ellipsis unless you're rethrowing (and possibly picking
> > up the exception again, but anyway rethrowing) within the catch
> > handler.
>
> Shouldn't you catch(...) on a module boundary
> (like at the main function of a thread), just to
> ensure no exceptions cross into a possibly non-C++
> area (like the OS).
>
> Or would your answer be "You should know what
> exceptions can get to that point, and catch
> all of them explicitly"?
The key question is "What do you propose to do afterwards?"
You probably shouldn't attempt to continue normal operation because
unless you know what the exception means any thing that you do is
potentially undefined behaviour.
An exception is thrown because things are not as you believed that
they had to inevitably be.
If you have independent "systems" then you might plausibly attempt to
continue with the other systems but that is not normally the case.
For the threading example I have written code to catch ... but the
only thing that it can logically do at that level is terminate.
If your thread is doing something unimportant (e.g a logging function)
then you might reasonably attempt to continue without it but then you
have to design your whole app around that - for example any attempt at
logging might now block indefintely if the thread held a mutex and
these are not released on theread death with your system. So unless
you are programming a pacemaker or a space probe termination is almost
certainly preferable since you must always consider unexpected
termination BEFORE giving any thought to catching unknown exceptions
since it is a more general failure mode.
Alf P. Steinbach
> "Alf P. Steinbach" <al...@start.no> wrote in
> news:he5rst$570$1...@news.eternal-september.org:
>
>>
>> Don't catch by ellipsis unless you're rethrowing (and possibly picking
>> up the exception again, but anyway rethrowing) within the catch
>> handler.
>
> Shouldn't you catch(...) on a module boundary
> (like at the main function of a thread), just to
> ensure no exceptions cross into a possibly non-C++
> area (like the OS).
>
> Or would your answer be "You should know what
> exceptions can get to that point, and catch
> all of them explicitly"?
It depends. :-)
The advice I gave is generally applicable within C++ code, but a module/language
boundary can be something else.
One technique, made easier with C++0x, is when an exception is caught at the
highest level of a callback invoked from C code (say), to store a clone of the
exception object and translate to an error return value, then rethrow at the
originally calling C++ side of things again.
At the outermost call level of the main function of a thread, if you have any
exception propagating up here then in general the thread has failed. Dealing
properly with that depends on your threading library. A reasonable way, if it is
supported, is to set the thread's exit code to EXIT_FAILURE and in some way make
available a clone of the exception object.
Hm, it seems that in C++98 exception object should always be clonable. :-)
Still, catching by ellipsis is generally ungood without rethrowing, because it
catches all sorts of exceptions, even, with old MSVC, operating system
exceptions like integer division by zero or nullpointer exception -- which
effectively was a language extension, but one worth keeping in mind. Not to
mention, as I believe that I did, pointer exceptions like in MFC, leaking
memory. So, since suppressing catastrophic failures and leaking memory is
ungood, catching by ellipsis without rethrowing is generally ungood.
Cheers & hth.,
- Alf
--
Eric J. Holtman
news:77ea554d-a3ee-4a06...@p23g2000vbl.googlegroups.com:
>
> The key question is "What do you propose to do afterwards?"
>
> You probably shouldn't attempt to continue normal operation because
> unless you know what the exception means any thing that you do is
> potentially undefined behaviour.
Oh, I know I can't do much there......
mostly just wanted to hit the log to say
"can't happen trap".