Using goto in c++ code

[Ergodyne]

This may be a hot topic but I am interested in knowing what the general thought about this is. When I started to learn programming I was taught to never use goto's in the code (c++) and that this was an old way of programming.

Today, I'm working with an old c/c++ application that uses this goto command quite a lot and so I'm starting to wonder if it is not such a bad idea, at least in this case.

As an example of how it is used



int SomeFunction(int parameter)
{
int retVal = FALSE;

if (!Initiate())
goto ERROR_END;

if (!AllocateMemory())
goto ERROR_END;

if (!RunAnalysis())
goto ERROR_END;

return TRUE;

ERROR_END:

ReleaseMemory();
return FALSE;

}



In this case it is to me quite useful and makes the code more readable. Of course one could switch this to a series of if else statements but to me it would make the code more incomprehendable.

Any comments on this?

[Ergodyne]

Ok,

just to document the answer to this question I will put this here.

I read a few sections in the book Code Complete and found that it was actually one of the cases where a goto statement could be at its best use. That is, when performing error checks and clean up after an error.

One alternative to this is to use try-catch statements, but since I cannot rewrite the code that I already have I will keep using the goto standard that is used.

[Paul McKenzie]

Today, I'm working with an old c/c++ Which is it, C or C++?

Also, you should have posted this in the non-Visual C++ forum. Nothing in your code suggests anything to do with the Windows API.

Who releases the memory when RunAnalysis() returns? Does Initiate() allocate memory? If not, does ReleaseMemory() make sense when you did a "goto" from Initiate()?

We need more context as to what those functions do, as the goto's would be totally unnecessary with correct usage of RAII.

Regards,

Paul McKenzie

[JohnW@Wessex]

I've been programming in C for 12 years then C++ for 9.
I used goto once (and I mean one goto) when I started learning C and I've never had reason to use it since.

[JVene]

I must echo these last two posts.

RAII solves the issue without goto in this example, and is even more reliable ongoing. If someone comes along to expand this code, it is up to them to understand why the ReleaseMemory is called. An object implementing RAII would handle this without fail.

Notice, too, how retval isn't used. I smell an oversight in design - as in I see a ReleaseMemory, but where was it allocated in the first place?

I also see no clear indication of even a weak guarantee; exception safety is more than questionable, probably non-existent.


In 29 years (I have to keep updating my count) I don't recall using goto outside of example.


I don't think the keyword or what it does is the problem. Goto is implied by nearly every close backet "}" character. C was originally design as a kind of assembler, and branching is simply a part of the way CPU's work.

I think the reason a vast majority of seasoned developers loath goto is what happens in the context of it's use. I've never found it to increase speed, utility or readability (though I agree it doesn't always impede any), but I've also found it was never required.

Famous case point is Knuth's use of goto in a performance illustration. It's cited around here in the last month or so. A loop, using goto, runs faster based on a simple optimization - it's occasionally used as an example in defense of goto. The pseudo code is in Pascal notation. However, even in this case, the reason the loop is faster is because the increment of the loop is advanced by 2 each time, not by 1, and the test runs on two consecutive elements in the loop. The optimization works as well inside a for loop, without goto - and the code even looks clearer.

[nuzzle]

Any comments on this?

Use goto to your heart's desire.

There's still quite a few of us who think the jump is coming to us. Heil.

[ruderudy]

It is good programming style to use the break, continue, and return statement in preference to goto whenever possible. Since the break statement only exits from one level of the loop, a goto may be necessary for exiting a loop from within a deeply nested loop.

But why would you want to ReleaseMemory if the Memory aint allocated?
It would have been nice to see what the functions does too.
I see that if AllocateMemory returns true it must be Released after RunAnalysis, i guess, if not then you dont have to release any memory.

this is your function without the goto statement:

int SomeFunction()
{
if (!Initiate() || !AllocateMemory() || !RunAnalysis())
{
ReleaseMemory();
return FALSE;
}
return TRUE;
}

[srelu]

I don't think your sample is the best, because in your case you can write simply:

int SomeFunction(int parameter)
{
if (Initiate())
if (AllocateMemory())
if (RunAnalysis())
return TRUE;
ReleaseMemory();
return FALSE;
}

What you suggest is more intrresting in the case when multiple objects must be destroyed. Let's suppose you need

CreateObject1
CreateObject2
CreateObject3

In this case, if CreateObject1 fails, you simply return error.
If CreateObject2 fails, before returning error, you must destroy Object1.
If CreateObject3 fails, beforre returning error, you must destroy Object1 and Object2.
You can achieve that using the goto statement as follows:

if(!CreateObject1)
goto label1
if(!CreateObject2)
goto label2
if(!CreateObject3)
goto label3
return TRUE; // success

label3: DestroyObject2;
label2: DestroyObject1;
label1: return FALSE; // failure


But you can achieve the same behaviour without using goto:

{
int iErr;
if(!CreateObject1)
iErr = 1;
else if(!CreateObject2)
iErr = 2;
else if(!CreateObject3)
iErr = 3;
else
return TRUE;

switch(iErr)
{ // break statements intentionally ommitted
case 3: DestroyObject2;
case 2: DestroyObject1;
case 1: return FALSE;

}
}

The point is that goto statements are not necessary, although you can use them. Some people will accept them. But in the case of others, the goto statement will raise eyebrows. My advice is to avoid it. You can.

[zaryk]

Most teachers that I have run into, and even some books say that using goto creates spaghetti code and is looked down upon.

[JVene]

.. goto creates spaghetti code...


I've heard this a lot, and in defense of the goto fans, I'd counter that it promotes it, but then I tend to be diplomatic about it.

[ruderudy]

I don't think your sample is the best, because in your case you can write simply:

int SomeFunction(int parameter)
{
if (Initiate())
if (AllocateMemory())
if (RunAnalysis())
return TRUE;
ReleaseMemory();
return FALSE;
}



If you look at his code this is exactly what he wants.
I was just suggesting a simpler solution.

Anyway, back to coding :)

[Odiee]

Use it or don't use it, it's up to the programmer. The use of goto in OP's code would be ok by me if the function in which it's used is carefully written and not longer then...say 50 lines? And don't use it if more then one person works on the code.
I used it few weeks ago in some settings wizard I wrote where was essential that user can click back or next without any data loss, but that was for fun.

[nuzzle]

This may be a hot topic but I am interested in knowing what the general thought about this is. When I started to learn programming I was taught to never use goto's in the code (c++) and that this was an old way of programming.


Not using goto is the theoretical foundation of high-level languages. It was proved that any program using goto could be replaced with a completely equivalent program without goto if a minimal set of control structures were used insted (like if-then-else, while-do, sequence, procedure, etcetera). This is also called the structured programming paradigm,

http://en.wikipedia.org/wiki/Structured_programming

This was very controversial at first. Many long-time assembly programmers felt that a language with just a few control structures would severely limit their right to freely express themselves. Others argued that it was worth it because restricting the programmer would reduce program complexity and thus lead to easier to understand and less error-prone programs. There was a very heated and infected battle and that's why the goto question is controversial even today. (old-timers in their late fifties can turn crimson red if they spot a goto because they get flashbacks from their youth when they put their lives on line in the war against the bogus goto).

As we know, structured programming and high-level languages won out eventually. Still the goto is there, both directly and in disguised form. Break, continue and multiple returns must be considered gotos. At least if you ask a hard-core anti-goto activist. It's because a goto-free control structure has exactly one entry and one exit point and those statements break that rule. Most people today consider them a very benign and accepted form of goto though.

So when is it okay to use an explicit goto? Well, the reason not to is that it increases the complexity of your code. It always does. So what you must ask yourself is whether it's worth it. Does the benefit of using goto offset the risks associated with more complex code? In my view it rarely does. The one time it's motivated is to handle errors, but then there's another systematic way to do that and that's to use exceptions. See Error Handling and Exceptions in the excellent book by Sutter & Alexandrescu called C++ Coding Standards.

[Kheun]

The most important thing to me is code readability. As long as any approach generate a clean code, I will use it. In C++, I will never thought of using goto at all because the language has better coding style. However, there are some situations in C, using goto will give a cleaner code.


int SomeFunction(int parameter)
{
if (!Initiate()) goto ERROR_END;
foo1();

if (!AllocateMemory()) goto ERROR_END;
foo2();

if (!RunAnalysis()) goto ERROR_END;
foo3();

return TRUE;

ERROR_END:
ReleaseMemory();
return FALSE;
}


If I am not using goto, the code would require some replications and looks more complicated than the above example.


int SomeFunction(int parameter)
{
if (!Initiate()) {
ReleaseMemory();
return FALSE;
}
foo1();

if (!AllocateMemory()) {
ReleaseMemory();
return FALSE;
}
foo2();

if (!RunAnalysis()) {
ReleaseMemory();
return FALSE;
}
foo3();

return TRUE;
}

[DeniskaB]

One of many possibilities:

int SomeFunction(int parameter)
{
int retVal = TRUE;

if (!Initiate()) retVal = FALSE;
else if (!AllocateMemory()) retVal = FALSE;
else if (!RunAnalysis()) retVal = FALSE;

if(!retVal) ReleaseMemory();
return retVal;
}

[JVene]

What occurs to me is that it seems obvious, without any further explication on the matter, that releasing memory does not happen unless an allocation is performed.

This is C++, right?

Here's another approach plan


int SomeFunction(int parameter)
{
if ( Initiate() )
{
foo1();
shared_ptr<t> p( AllocateMemory() );

if ( p )
{
foo2();

if ( RunAnalysis() )
{
foo3();
return true;
}
}

}

return false;
}




Or if your coding standard is strict regarding only one return place


int SomeFunction(int parameter)
{
bool rval = false;

if ( Initiate() )
{
foo1();
shared_ptr<t> p( AllocateMemory() );

if ( p )
{
foo2();

if ( RunAnalysis() )
{
foo3();
rval = true;
}
}

}

return rval;
}




And it seems reasonable that 3 indentations is at or approaching that "enough" limit, plus the rest may likely be better inside type t:



int SomeFunction(int parameter)
{
if ( Initiate() )
{
foo1();
shared_ptr<t> p( AllocateMemory() );

if ( p )
{
return p->Process( parameter );
}

}

return false;
}




bool t::Processing( int parameter )
{
bool rval = false;

foo2();

if ( RunAnalysis() )
{
foo3();
rval = true;
}

return rval;
}


In this latter case, foo2 and foo3 may well be part of class t, as they are only performed if things are going well, appear 'part of' what's happening with regard to t, at least after initializations occurred.


Now riddle me this:

What happens to allocated memory in the context of exceptions thrown by foo3, foo2 or RunAnalysis in the "goto" versions above?

And, what are your options about that?

..think about it...

[plong]

I'm a strict one-entry-one-exit kind of guy, so the one-return snippet looks good to me. Extra returns are second cousins to gotos.

This is what I'd like to see:


int SomeFunction(int parameter)
{
int retVal = FALSE;

if (Initiate()) {
if (AllocateMemory()) {
if (RunAnalysis()) {
retVal = TRUE;
}
} else {
ReleaseMemory();
}
}

return retVal;
}


or the variable-less but cringe-worthy:


int SomeFunction(int parameter)
{
return Initiate() && (AllocateMemory() ? RunAnalysis() : ReleaseMemory(), FALSE);
}


I've always heard that 4 or 5 levels was the limit for nesting control structures, but a quick Google shows "3" and "3 to 5," so whatever.

[JVene]

Torvalds made a somewhat famous quote (or at least it's attributed to him), "If you have more than 3 indentations in your code, you need to fix your code".

I'm personally less convinced of the similarity between multiple returns and goto. I would agree it was more of an issue prior to C++ and without strict use of RAII, and I recall dealing with bugs associated with multiple returns in "the old days". The point of caution against it does have merit.

[JohnW@Wessex]

Torvalds made a somewhat famous quote (or at least it's attributed to him), "If you have more than 3 indentations in your code, you need to fix your code".
I'd be cautious about assuming that someone is a coding style guru, merely because they wrote a popular piece of software.

[binyo66]

I am agree with Kheun.
Lets compare this 2 functions, and lets assuming the caller
needs to get the exact returns value (0 no errors). As you
may know sometimes GetLastError function is not clear enough.

1)
long RunLnkFromPIDL(long pShExecInfo /*in out */)
{
HRESULT hres;
IShellLink* psl;
long lRet=0;
WORD *wsz=0;
SHELLEXECUTEINFO *pInfo=(SHELLEXECUTEINFO *)pShExecInfo;
if (!pInfo)
return -1;
pInfo->lpVerb = NULL;
// Get a pointer to the IShellLink interface.
hres = CoCreateInstance(CLSID_ShellLink, NULL,
CLSCTX_INPROC_SERVER, IID_IShellLink, (void **)&psl);
if (SUCCEEDED(hres)) {
IPersistFile* ppf;

// Get a pointer to the IPersistFile interface.
hres = psl->QueryInterface(IID_IPersistFile,
(void **)&ppf);
if (SUCCEEDED(hres)) {
wsz=new word [MAX_PATH];
if (wsz)
{ // failed to allocated
// Ensure that the string is Unicode.
lRet=MultiByteToWideChar(CP_ACP, 0, pInfo->lpFile, -1, wsz,
MAX_PATH);
if (SUCCEEDED(lRet)) {
// Load the shortcut.
hres = ppf->Load(wsz, STGM_READ);
if (SUCCEEDED(hres)) {

// Resolve the link.
assert(pInfo->hwnd);
hres = psl->Resolve(pInfo->hwnd, SLR_ANY_MATCH);
if (SUCCEEDED(hres)) {
//LPITEMIDLIST pidl=(LPITEMIDLIST *)*((long *)pidlAddr);
hres = psl->GetIDList((_ITEMIDLIST **)&(pInfo->lpIDList));
ShellExecuteEx(pInfo);

//GetModuleFileName(pInfo->hInstApp, sName, MAX_PATH);
}
else
lRet=-7;
}
else
lRet=-6;
}
else
lRet=-5
delete [] wsz;
}
else
lRet=-4;
// Release the pointer to the IPersistFile interface.
ppf->Release();
}
else
lRet=-3;
// Release the pointer to the IShellLink interface.
psl->Release();
}
else
return -2;

if (wsz)
delete [] wsz;
return lRet;
}

2)
long RunLnkFromPIDL(long pShExecInfo /*in out */)
{
HRESULT hres;
IShellLink* psl;
long lRet=0;
WORD *wsz=0;
IPersistFile* ppf;
SHELLEXECUTEINFO *pInfo=(SHELLEXECUTEINFO *)pShExecInfo;
if (!pInfo)
return -1;
pInfo->lpVerb = NULL;
// Get a pointer to the IShellLink interface.
hres = CoCreateInstance(CLSID_ShellLink, NULL,
CLSCTX_INPROC_SERVER, IID_IShellLink, (void **)&psl);
if (FAILED(hRes))
return -2;

// Get a pointer to the IPersistFile interface.
hres = psl->QueryInterface(IID_IPersistFile,
(void **)&ppf);
if (FAILED(hRes) {
lRet=-3;
goto END;
}
wsz=new word [MAX_PATH];
if (!wsz) {
lRet=-4;
goto END1;
}
// Ensure that the string is Unicode.
lRet=MultiByteToWideChar(CP_ACP, 0, pInfo->lpFile, -1, wsz,
MAX_PATH);
if (FAILED(lRet)) {
lRet=-5;
goto END1;
}
// Load the shortcut.
hres = ppf->Load(wsz, STGM_READ);
if (FAILED(hRes)) {
lRet=-6;
goto END1;
}
// Resolve the link.
assert(pInfo->hwnd);
hres = psl->Resolve(pInfo->hwnd, SLR_ANY_MATCH);
if (SUCCEEDED(hres)) {
//LPITEMIDLIST pidl=(LPITEMIDLIST *)*((long *)pidlAddr);
hres = psl->GetIDList((_ITEMIDLIST **)&(pInfo->lpIDList));
ShellExecuteEx(pInfo);

}
else
lRet=-7;
END1:
ppf->Release();
END:
psl->Release();
if (wsz)
delete [] wsz;
return lRet;
}

I think 2 is more readable. And, IMO, one the main purpose of structural programming
language is to make code more readable, so that it is easy to maintain. therefore, I am
still using goto to clean up local variables. BTW this is probably more C rather
than C++. And, sorry, I just paste from my code, and copy to notepad, it really looks ugly, all spaces gone :(. I havent post a lot, so I dont know how to make it space yet.

[Kheun]

I don' think I will ever use goto in C++ as there are multiple ways to avoid it and still writing clean code. However, it will be a different story when the programming language of choice is C. It is especially so for embedded software where the device memory footprint is small. Even though there had been many advances in C++ compiler development, the minimum memory footprint requirement for C++ is much larger than C.

[Arjay]

Using goto is okay in C++ if you can't figure out any other way to do it.

Though, I haven't happen to run across any cases where it was necessary, but I would kind of like to see a case where it was the only alternative.

[Paul McKenzie]

Using goto is okay in C++ if you can't figure out any other way to do it.I don't necessarily agree.

If the coder isn't very good, then the excuse they will use for using goto is that they couldn't figure out a way. Of course, there is always a way around "goto".

Regards,

Paul McKenzie

[Arjay]

I don't necessarily agree.

If the coder isn't very good, then the excuse they will use for using goto is that they couldn't figure out a way. Of course, there is always a way around "goto".

Regards,

Paul McKenzieThat's my point. There's no need to use goto.

:wave: