Sign in to follow this  
Storyyeller

Use of goto in C++?

Recommended Posts

Storyyeller    215
Everyone always says that you shouldn't just always avoid goto, instead you should learn when it is appropriate and not appropriate to use. So what are the legitimate uses of goto? I can't think of any.

Share this post


Link to post
Share on other sites
Deyja    920

while (whatever)
{
for (some; crap; here)
{
goto OutsideTheLoop;
}
}

OutsideTheLoop :


In C++, goto can be used to implement multi-level break.

Share this post


Link to post
Share on other sites
Antheus    2409
Use of Windows or Linux API which is C. Some of the idioms of these APIs are still simplest to express via goto, since bringing in either RAII or exceptions opens a whole different can of worms.

Share this post


Link to post
Share on other sites
KieranW    100
while (whatever)
{
for (some; crap; here)
{
goto OutsideTheLoop;
}
}

OutsideTheLoop :


This is ugly. You should rather use:

while (!done)
{
for (some; crap; here)
{
done = true;
break;
}
}


This will set done to true, and then break from the for loop, which in turn, will break the while loop. There is no need to use goto here.

Share this post


Link to post
Share on other sites
Zahlman    1682
while (whatever && !done)

:)

Seriously, though: practically speaking, you don't have a reason to use goto. If you think you have a reason to use goto, show me the code and I'll give you a better idea.

(Often, the best solution for multi-level break is to refactor the loops into their own function and return from the function.)

Share this post


Link to post
Share on other sites
KieranW    100
Lol thanks for correcting me. That is a better way of doing it if you also require another condition.

And I agree. There is no need for goto. I don't remember the last time I used it.

Share this post


Link to post
Share on other sites
marsong    154
Quote:
Original post by KieranW
while (whatever)
{
for (some; crap; here)
{
goto OutsideTheLoop;
}
}

OutsideTheLoop :


This is ugly. You should rather use:

while (!done)
{
for (some; crap; here)
{
done = true;
break;
}
}


This will set done to true, and then break from the for loop, which in turn, will break the while loop. There is no need to use goto here.


The goto looks much cleaner and easier to read to me, especially if you have a reasonably named label. If there's a lot of code between the while and the for, or maybe even a third level, then you are gonna need to go back up, check what the done = true will do to the loop, and what if you have code after the for loop that you also wanna jump over?

The "evilness" of goto comes from jumping back and forth and all over the place, if all you're doing is jumping "down" over a bunch of code it is at least as clean as a break or a function call.

Share this post


Link to post
Share on other sites
KieranW    100
[QUOTE]The goto looks much cleaner and easier to read to me. If there's a lot of code between the while and the for, or maybe even a third level, then you are gonna need to go back up, check what the done = true will do to the loop, and what if you have code after the for loop that you also wanna jump over?[/QUOTE]

To everyone else, the other way looks cleaner.

It's obvious what done = true will do to the loop. It will break it because it's a while loop with the condition while(!done) which means as soon as done is set to true, the loop finishes. All the code after the for loop will not be executed once done = true, because the while loop will break as well and it will skip to the code after the loop. It essentially does the same thing as the goto did. If there is a third level, give that the condition of !done as well and it wall also break.

There is a solution to everything that is much cleaner than a goto.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
Original post by Zahlman

Seriously, though: practically speaking, you don't have a reason to use goto. If you think you have a reason to use goto, show me the code and I'll give you a better idea.


Example taken from MSDN, quickly fixed for in this editor, but hopefully it gets the picture across.
HBITMAP CopyBitmap( HBITMAP hbm) {
HBITMAP hbmOld, hbmOld2, hbmNew = 0;
BITMAP bm;

HDC hdcSrc = CreateCompatibleDC(NULL);
if (hdcSrc == NULL) goto errSrc;

HDC hdcDst = CreateCompatibleDC(NULL);
if (hdcDst == NULL) goto errDst;


GetObject(hbm, sizeof(bm), &bm);

hbmOld = SelectObject(hdcSrc, hbm);
if (hbmOld == NULL || hbmOld == GDI_ERROR) goto errOld;

hbmNew = CreateBitmap( bm.bmWidth, bm.bmHeight, bm.bmPlanes, bm.bmBitsPixel, NULL);
if (hbmNew == NULL) goto err;

hbmOld2 = SelectObject(hdcDst, hbmNew);
if (hbmOld2 == NULL || hbmOld == GDI_ERROR) goto err;

BitBlt(hdcDst, 0, 0, bm.bmWidth, bm.bmHeight, hdcSrc, 0, 0, SRCCOPY);

err:
SelectObject(hdcSrc, hbmOld);
errOld:
DeleteDC(hdcDst);
errDst:
DeleteDC(hdcSrc);
errSrc:
return hbmNew;
}


Obviously there are other ways, and hopefully the WinAPI will make people cringe, and one could argue that it applies to C portion of C/C++ used for Windows development. Still...

Share this post


Link to post
Share on other sites
KieranW    100
2 ways to fix it.

1. Make functions for each of the errors
2. Place those functions inside the if statements instead of after a goto label

There are other ways which are also better, but i'm not going to go over all of them.

Share this post


Link to post
Share on other sites
bobofjoe    322
Quote:
Original post by Antheus
Quote:
Original post by Zahlman

Seriously, though: practically speaking, you don't have a reason to use goto. If you think you have a reason to use goto, show me the code and I'll give you a better idea.


Example taken from MSDN, quickly fixed for in this editor, but hopefully it gets the picture across.
*** Source Snippet Removed ***

Obviously there are other ways, and hopefully the WinAPI will make people cringe, and one could argue that it applies to C portion of C/C++ used for Windows development. Still...




//Assume scoped handle acts kinda like shared_ptr.

HBITMAP CopyBitmap( HBITMAP hbm) {
HBITMAP hbmOld2, hbmNew = 0;
BITMAP bm;

scoped_handle<HDC> hdcSrc(CreateCompatibleDC(NULL), DeleteDC);
if (!hdcSRC) { return NULL; }

scoped_handle<HDC> hdcDst(CreateCompatibleDC(NULL), DeleteDC);
if (!hdcDst) { return NULL; };

GetObject(hbm, sizeof(bm), &bm);

scoped_handle<HBITMAP> hbmOld(SelectObject(hdcSrc, hbm), bind(SelectObject, hdcSrc, _1));
if (*hbmOld == NULL || *hbmOld == GDI_ERROR) return NULL;

hbmNew = CreateBitmap( bm.bmWidth, bm.bmHeight, bm.bmPlanes, bm.bmBitsPixel, NULL);
if (hbmNew == NULL) return NULL;

hbmOld2 = SelectObject(hdcDst, hbmNew);
if (hbmOld2 == NULL || hbmOld == GDI_ERROR) return NULL;

BitBlt(hdcDst, 0, 0, bm.bmWidth, bm.bmHeight, hdcSrc, 0, 0, SRCCOPY);

return hbmNew;
}



Share this post


Link to post
Share on other sites
benryves    1999
Quote:
Original post by KieranW
And I agree. There is no need for goto. I don't remember the last time I used it.
There's no need for control structures beyond if statements and goto. [wink] Use whatever's appropriate.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
//Assume scoped handle acts kinda like shared_ptr.


You just required me to download STLPort, or implement scoped_handle myself. Use of templates might also cause it to not compile under VC6.

This also require STLPort to be included into build, or my own version must be tested under VC6 SP4 which is only compiler that can build the application, yet I do not have VC6 at all.

The simple old C code (part of C++ application) is now C++ code with increased compilation times and footprint. This may require function to be moved outside of pure C extern namespace into C++ one. Rearranging the code in this way may break the includes and build process due to implicit dependency in order of includes.

It also adds bind, which means that 55496 headers will be now included instead of *one single line*.

In addition, there is now inconsistency - scoped_handle suddenly appeared in code which nobody else knows or cares about.

It may also require extra commit to repository, but I do not have permission to put it into utils or shared, so I need to stick it either in the current file, or as part of bitmap routines. Worst thing that could happen is that someone will find it useful, and start including my bitmap routines.

Also, scoped pointer requires, depending on which resource was obtained, a specialized, somethings per-call specific handle release routine (depending on whether it's shared, reference counted, in DLL), which means another functor which describes destruction process.

Keep in mind - WinAPI development is legacy stuff, applications are old and depend on million of factors, none of which are technical.

Share this post


Link to post
Share on other sites
Promit    13246
It's like anything else. Anybody speaking in absolutes is wrong, end of story. The correct refactor for a goto is a new function in nearly all cases, but sometimes the goto is just plain nicer. Modern code with goto typically enforces the "down-only" restriction, which helps a lot.

Share this post


Link to post
Share on other sites
voodoohaust    122
Honestly, just DON'T use goto :)
Ok, goto is part of C/C++ you can use it but be able to doesn't mean have to....

I know some people will always find/give a special case where goto could/should be used.

But as soon you think you have a valid reason to use this keyword you'll find another one and then another, etc. In the end your code will be goto-land.

For every goto use there's always an equivalent that doesn't use it :)

Share this post


Link to post
Share on other sites
Daaark    3553
Nothing wrong with using goto. It works as advertised, is fully supported in almost every language, even new ones. Just don't abuse it. You can do a lot of stupid things with goto, and 9 times out of ten there is a better solution.

On that tenth time, when that "better solution" is to screw around with simple, easy to read code that works and make something less clear just to simulate what goto already does anyways!

I use it as a nice clean way to break, or jump to different parts of a complicated multilevel loop I use to generate some game content. I find other, over-engineered solutions around this problem laughable. Don't build a tank to solve what a water gun can. And flow control in nested loops is the only place I have ever seen them to be a good use in like 20 years.

Just make sure to set some ground rules. Like Promit said, down only! But also keep it in the same function. Make sure you understand exactly what code you are going to skip over. So you don't end up not executing code that needs to set something up or de-allocate something.

Share this post


Link to post
Share on other sites
Zahlman    1682
scoped_handle - or else a more task-specific variant - isn't so hard to roll yourself. But assuming you have to do it in C, actual analysis of the code structure helps. Use some helper functions to wrap the special behaviour of NULL handles, separate resource acquisition (CreateCompatibleDC/DeleteDC) from the main body of work, and don't be afraid of a little nesting in that part. (The careful reader will note that I haven't separated out all the resource acquisition; CreateBitmap() is also resource creation. Fixing this is left as an exercise. ;) )

I might have missed something subtle, or just typo'd somewhere, but something like this should do the trick:


// Doesn't DeleteDC already handle this? If not, why not?
void CleanupDC(HDC hdc) {
if (hdc != NULL) { DeleteDC(hdc); }
}

// Attempt to select a non-NULL HBITMAP into a non-NULL HDC. If anything is
// wrong, return NULL; otherwise return the SelectObject result.
HBITMAP GuardedSelectBitmap(HDC hdc, HBITMAP bm) {
HBITMAP result;
if (hdc == NULL || bm == NULL) { return NULL; }
result = SelectObject(hdc, bm);
if (result == GDI_ERROR) { result = NULL; }
return result;
}

// Given non-NULL handles for blank src and dst DCs, use those as scratch space
// to copy the bitmap and return a handle to the copy.
HBITMAP CopyBitmapHelper(HBITMAP hbm, HDC src, HDC dst) {
HBITMAP copy, backup = GuardedSelectBitmap(src, hbm);
BITMAP bm;

// Check that we swapped the bitmap into the src DC.
if (backup == NULL) { return NULL; }

// Measure the bitmap and create a blank bitmap of the same size.
GetObject(hbm, sizeof(bm), &bm);
copy = CreateBitmap(bm.bmWidth, bm.bmHeight, bm.bmPlanes, bm.bmBitsPixel, NULL);

if (copy != NULL) {
// If we're OK so far, try to swap the blank into the dst DC and then
// blit onto it from the src.
if (GuardedSelectBitmap(dst, copy) != NULL) {
BitBlt(dst, 0, 0, bm.bmWidth, bm.bmHeight, src, 0, 0, SRCCOPY);
}
// Shouldn't we destroy the copy and return NULL if we fail to select
// it into dst? The original code didn't do that, though...
}

// We know src and backup are non-NULL if we get here :)
SelectObject(src, backup);
return copy;
}

HBITMAP CopyBitmap(HBITMAP hbm) {
HDC src = CreateCompatibleDC(NULL);
HDC dst = CreateCompatibleDC(NULL);
// CopyBitmapHelper will fail fast and safely if either allocation failed.
HBITMAP result = CopyBitmapHelper(hbm, src, dst);
CleanupDC(src);
CleanupDC(dst);
return result;
}

Share this post


Link to post
Share on other sites
TheGroggyOne    100
There's nothing wrong with goto/labels, there's no right or wrong time to use them.

But consider a program with 100,000 lines of code, 10,000 gotos, say 8,000 labels that can be named anything. Then you have to follow the program flow across 30 files, through all the if statements that branches to goto, all the select statements that branch with goto and all the goto's inside of the labels, then all the goto and labels that should've been taken out and wasn't, then add in the fact it was written by 20 other people over the course of 10yrs.

If the code you're writing isn't going to be seen by anyone else, then go goto happy. If it is, then keep the flow of the program simple. Go to a function, then come back from it.

Goto/labels are leftovers from low level languages. It's an exotic programming style, only used by eggheads and professors, these days. It still has its niche, but not in mainstream programming. If you ever do any assembly or microprocessor programming, you'll understand it better.

Share this post


Link to post
Share on other sites
M2tM    948
The example of a multiple level break was a good idea, but instead of a while loop, let's assume we have two for loops in this fictional code snippet, further let us introduce something to process just after the inner for loop. Let us also assume there is always going to be a single enemy in a grid:


...
for(x = 0;x < 100;x++){
for(y = 0;y < 100;y++){
if(Grid[x][y] == ENEMY_PLAYER){
goto loopBreak;
}
}
processEmptySquare(x, y);
}
loopBreak:
...






In this code sample we clearly want a multi-level break, the goto expresses this very clearly and is, in my opinion, one of the rare cases where it is more readable to use than to try and avoid. The problem is that if we want to introduce a done flag it would cause something like the following:


...
bool done = false;
for(x = 0;x < 100 && !done;x++){
for(y = 0;y < 100;y++){
if(Grid[x][y] == ENEMY_PLAYER){
done = true;
}
}
if(!done){
processEmptySquare(x, y);
}
}
...






I believe this example is harder to follow as it introduces the added mental overhead of not exiting the loop immediately (causing the need to watch what you do after the inner loop) and of disconnecting the exit flag from the exit condition. It also requires two additional decisions every iteration of x, probably not a big performance trouble, but messier.

Here I believe the goto is actually the more elegant solution. This type of situation is relatively rare, but not unheard of.

Of course, you could argue that the code should be re-written:


bool EnemyPlayerExistsInRow(const int x, const std::vector<int> &Grid, int &yReturn){
for(yReturn = 0;yReturn < 100;yReturn++){
if(Grid[x][yReturn] == ENEMY_PLAYER){
return true;
}
}
return false;
}

...
for(x = 0;x < 100 && !EnemyPlayerExistsInRow(x, Grid, y);x++){
processEmptySquare(x, y);
}
...






This is, of course, a better solution as it self-documents with the function and presents less mental overhead when reading the loop, a layer of abstraction has rightly been moved into a function. The exit condition is also no longer strewn drunkenly throughout the entire looping structure in the form of a done flag.

I am also assuming that this loop would be a part of a compact function, but I leave that up to your imagination.

[Edited by - M2tM on February 19, 2010 5:32:17 AM]

Share this post


Link to post
Share on other sites
Guest
This topic is now closed to further replies.
Sign in to follow this