tags:

views:

1972

answers:

33
+4  A: 

In general, you should design your programs to limit the need for gotos. Use OO techniques for "cleanup" of your return values. There are ways to do this that don't require the use of gotos or complicating the code. There are cases where gotos are very useful (for example, deeply nested scopes), but if possible should be avoided.

Marcin
Deeply nested scopes can be refactored out into a separate function, and then a single return statement easily replaces your goto there as well. :)
jalf
+28  A: 

I am not sure what do you mean by clean up code but in C++ there is a concept called "resource acquisition is initialization" and it should be the responsibility of your destructors to clean up stuff.

(Note that in C# and Java, this is usually solved by try/finally)

For more info check out this page: http://www.research.att.com/~bs/bs_faq2.html#finally

EDIT: Let me clear this up a little bit.

Consider the following code:

void MyMethod()
{
    MyClass *myInstance = new MyClass("myParameter");
    /* Your code here */
    delete myInstance;
}

The problem: What happens if you have multiple exits from the function? You have to keep track of each exit and delete your objects at all possible exits! Otherwise, you will have memory leaks and zombie resources, right?

The solution: Use object references instead, as they get cleaned up automatically when the control leaves the scope.

void MyMethod()
{
    MyClass myInstance("myParameter");
    /* Your code here */
    /* You don't need delete - myInstance will be destructed and deleted
     * automatically on function exit */
}

Oh yes, and use auto_ptr or something similar because the example above as it is is obviously imperfect.

DrJokepu
It helps in cleanup code , plus it also helps in avoiding multiple return paths from routine that are some times very difficult to track .
Alien01
ALien01: Multiple return paths and multiple, "goto end of function" are exactly equal in difficulty to track.
Brian
The above code is in C, so C++ methods doesn't seem to apply
chmike
Oops, checked again and it is indeed ment to be C++. So indeed better make use of RIAA.
chmike
+3  A: 

Using goto to go to a cleanup section is going to cause a lot of problems.

First, cleanup sections are prone to problems. They have low cohesion (no real role that can be described in terms of what the program is trying to do ), high coupling (correctness depends very heavily on other sections of code), and are not at all exception-safe. See if you can use destructors for cleanup. For example, if int *p is changed to auto_ptr<int> p, what p points to will be automatically released.

Second, as you point out, it's going to force you to declare variables long before use, which will make it harder to understand the code.

Third, while you're proposing a fairly disciplined use of goto, there's going to be the temptation to use them in a looser manner, and then the code will become difficult to understand.

There are very few situations where a goto is appropriate. Most of the time, when you are tempted to use them, it's a signal that you're doing things wrong.

David Thornley
okay what about when we want to avoid multiple return paths from routing and also want to return some error value.Goto easily avoid multiple return paths and we can set error value in goto section . That way we never miss setting return error value.
Alien01
You think multiple return paths are a problem, and gotos are not?
David Thornley
Why do you consider multiple return paths a problem? And if you don't declare the return value ahead of time, you never forget setting it either. (instead of bRetVal = FALSE at the top of the function, just return true/false directly.
jalf
+11  A: 

Probably not a good idea.

Marc Charbonneau
what i was going to link to
Devoted
me too.. I have this printed out and posted in my cube. Fear the velociraptor.
FryGuy
+1  A: 

All of the above is valid, you might also want to look at whether you might be able to reduce the complexity of your code and alleviate the need for goto's by reducing the amout of code that is in the section marked as "lot of code" in your example. Additionaly delete 0 is a valid C++ statement

Harald Scheirich
+12  A: 

Your code is extremely non-idiomatic and you should never write it. You're basically emulating C in C++ there. But others have remarked on that, and pointed to RAII as the alternative.

However, your code won't work as you expect, because this:

p = new int;
if(p==NULL) { … }

won't ever evaluate to true (except if you've overloaded operator new in a weird way). If operator new is unable to allocate enough memory, it throws an exception, it never, ever returns 0, at least not with this set of parameters; there's a special placement-new overload that takes an instance of type std::nothrow and that indeed returns 0 instead of throwing an exception. But this version is rarely used in normal code. Some low-level codes or embedded device applications could benefit from it in contexts where dealing with exceptions is too expensive.

Something similar is true for your delete block, as Harald as said: if (p) is unnecessary in front of delete p.

Additionally, I'm not sure if your example was chose intentionally because this code can be rewritten as follows:

bool foo() // prefer native types to BOOL, if possible
{
    bool ret = false;
    int i;
    // Lots of code.
    return ret;
}
Konrad Rudolph
New operator will return NULL.Checkout the link: http://msdn.microsoft.com/en-us/library/kftdy56f(VS.71).aspx
Alien01
No, it *won't*. That article uses a different `new` overload, namely nothrow operator new. This is what I referred to in my above answer. I'll try to clarify that further.
Konrad Rudolph
actually, the code emulates Fortran, rather than C. There is no reason for using GOTO in C eiter.
andreas buykx
@andreas: Admittedly, I'm not a C programmer. How do you handle errors and complex cleanup logic in C without resorting to `goto` or convoluted, nested `if` blocks?
Konrad Rudolph
A: 

try it this way

BOOL foo()
{
   BOOL bRetVal = FALSE;
   int *p=NULL;

   p = new int;
   if(p==NULL)
   {
     cout<<" OOM \n";
   }
   else
   {
   // Lot of code...
   }

   if(p)
   {
     delete p;
     p= NULL;
   }
   return bRetVal;
}

In the section "Lot of code", "Lot of code" is a good indication that you probably should refactor this section into one or more methods or functions.

Matthew
This suffers from the same problems that have already been pointed out. Don't write code like that in C++. Not even low-level code.
Konrad Rudolph
@Konrad: I understand you take issue with the various tests on p, but I think that's a side issue. Matthew's suggestion is AFAIK the classic "correct" method for avoiding GOTO -- use real branching logic instead.
Dave Costa
I disagree @Konrad. It may not be out of the book, it may miss that delete NULL is valid, but OTOH the posters were wrong about exceptions on an OOM error (hint: set_new_handler()). And in no way is this post -1, this is ridiculous, so I will upvote. And the refactoring remark is very true aswell.
mstrobl
Konrad is right, this misses a fundamental point. Screw the tests on p, they're not important. What matters is that you're using simple, error-prone branching when C++ gives you much better tools for handling cleanup, such as RAII.
jalf
@mstrobl: The null test here was only a tiny part of my criticism. The whole code, as it stands, is an antipattern in C++. jalf understood me. Also, the “refactor” remark might be valid but I find it overrated. Some functions *do* get long in practice and there's nothing you can do to prevent it.
Konrad Rudolph
There's nothing you can do to *prevent* it, but you can *usually* fix it, too. In any case, it's something to seriously look at doing, before you start throwing in GOTOs and cleanup code.
Adam Jaskiewicz
As far as I see it, Mattew's solution is a good C solution (remplace new/delete by malloc/free). But this is a BAD C++ solution: No RAII (either stack or smart pointer), ignorance the fact a failing new is supposed to throw, etc.. Re-reading the question I see both use of "new" and tag "c++"...
paercebal
+2  A: 

The entire purpose of the every-function-has-a-single-exit-point idiom in C was to put all the cleanup stuff in a single place. If you use C++ destructors to handle cleanup, that's no longer necessary -- cleanup will be done regardless of how many exit points a function has. So in properly-designed C++ code, there's no longer any need for this kind of thing.

Head Geek
A: 

Using "GOTO" will change the "logics" of a program and how you enterpret or how you would imagine it would work.

Avoiding GOTO-commands have always worked for me so guess when you think you might need it, all you maybe need is a re-design.

However, if we look at this on an Assmebly-level, jusing "jump" is like using GOTO and that's used all the time, BUT, in Assembly you can clear out, what you know you have on the stack and other registers before you pass on.

So, when using GOTO, i'd make sure the software would "appear" as the co-coders would enterpret, GOTO will have an "bad" effect on your software imho.

So this is more an explenation to why not to use GOTO and not a solution for a replacement, because that is VERY much up to how everything else is built.

Filip Ekberg
+25  A: 

I've never had to use a goto in C++. Ever. EVER. If there is a situation it should be used, it's incredibly rare. If you are actually considering making goto a standard part of your logic, something has flown off the tracks.

PhoenixRedeemer
short and simple answer, but I think it nails it. goto's are in the language for a reason, certainly, but if you actually consider using them, there's a 99.9% chance that you're Doing It Wrong. Don't write your code as if it falls into the 0.01% case because it probably doesn't.
jalf
Sometimes you need a goto to break from nested loops.
rlbond
@rlbond: While this is often repeated, it's just as often proven to be false. (This is Tagged `C++`. C++ has inlining. So you can always use a `return` instead of a `goto`.)
sbi
@ribond: "want" != "need". Been there, done that - and as a rule of thumb, each `goto` seems to increase your code maintenance costs by 30%. Yes, yes, *you* understand the code *now*. That doesn't say anything about the programmer who will be modifying it in two years.
Piskvor
+1  A: 

Using GOTO labels in C++ is a bad way to program, you can reduce the need by doing OO programming (deconstructors!) and trying to keep procedures as small as possible.

Your example looks a bit weird, there is no need to delete a NULL pointer. And nowadays an exception is thrown when a pointer can't get allocated.

Your procedure could just be wrote like:

bool foo()
{
    bool bRetVal = false;
    int p = 0;

    // Calls to various methods that do algorithms on the p integer
    // and give a return value back to this procedure.

    return bRetVal;
}

You should place a try catch block in the main program handling out of memory problems that informs the user about the lack of memory, which is very rare... (Doesn't the OS itself inform about this too?)

Also note that there is not always the need to use a pointer, they are only useful for dynamic things. (Creating one thing inside a method not depending on input from anywhere isn't really dynamic)

TomWij
Make p an auto_ptr<int> or shared_ptr<int>, and you don't have to worry about whether the delete p; is ever executed.
David Thornley
Rewritten the answer, and besides that I'm not a boost user (yet) as we need to use the delete method at university. Code profilers can assist you when you forget to use delete somewhere in the code.
TomWij
Bad univerity, then. auto_ptr is in the standard library, and boost's shared_ptr will be in next year. Universities should not teach you to use delete, that's a technique from 10+ years ago. (It's OK to show delete when explaining how RAII works, of course)
MSalters
+6  A: 

In general, and on the surface, there isn't any thing wrong with your approach, provided that you only have one label, and that the gotos always go forward. For example, this code:

int foo()
{
    int *pWhatEver = ...;
    if (something(pWhatEver))
    { 
        delete pWhatEver;
        return 1;
    }
    else
    {
        delete pWhatEver;
        return 5;
    }
}

And this code:

int foo()
{
    int ret;
    int *pWhatEver = ...;
    if (something(pWhatEver))
    { 
        ret = 1;
        goto exit;
    }
    else
    {
        ret = 1;
        goto exit;
    }
exit:
    delete pWhatEver;
    return ret;
}

really aren't all that different from each other. If you can accept one, you should be able to accept the other.

However, in many cases the raii (resource acquisition is initialization) pattern can make the code much cleaner and more maintainable. For example, this code:

int foo()
{
    Auto<int> pWhatEver = ...;

    if (something(pWhatEver))
    {
        return 1;
    }
    else
    {
        return 5;
    }
}

is shorter, easier to read, and easier to maintain than both of the previous examples.

So, I would reccomend using the raii approach if you can.

Scott Wisniewski
Very good if your code does not use exceptions. Unfortunately C++ has exceptions so no. To make the code exception safe you must use RIAA.
Martin York
A: 

From Scott's comment It looks like using goto to jump from one section to another is bad as it make the code hard to read and understand.

But if we use goto just to go forward and to one label then it should be fine???

Alien01
No. `goto` is always bad if there are better solutions, and in your case (and, I argue, in all other cases as well) there are.
Konrad Rudolph
can you explain why still this is bad when its used to go forward and only to one label ,and this design being followed in whole project...I want to understand what amount of bad things it can cause....
Alien01
The exact behaviour you are describing is better identified and expressed by the statement " char* blub; { blub = new something; if (that) break; or_something_else(); } delete blub; /* clean up... */". But this is still lousy behaviour. :)
mstrobl
Your code is more robust if cleanup code is executed by the type being cleaned up, rather than by the caller. When your variable goes out of scope, it should clean itself up. That way you *know* it gets cleaned up. And you only have to implement the cleanup code *once*, not every time it's used.
jalf
Moreover, the mere existence of gotos makes your code harder to read.. How do I, when reading it, convince myself that the gotos *are* actually forward-only and to just one label? Only by checking the rest of the code for gotos too. So it still reads as spaghetti code, even if it isn't.
jalf
No. Still not EXCEPTION safe. You should be using RIAA for cleanup. Then your use of goto is not required. I have used goto once in 20 years. Also the way
Martin York
By the way, you cannot always go "forward", IIRC. For example, you are not supposed to bypass object construction code. Using goto is like emptying your toilets by hand. Sometimes, you MUST do it, and feel quite dirty because of it, but 99.99% the time, just flush it away...
paercebal
Martin, I believe you're referring to RAII ;-)
dancavallaro
+1  A: 

Since, this is a classic topic, I will reply with Dijkstra's Go-to statement considered harmful (originally published in ACM).

mstrobl
Please also read "Structured Programming with goto Statements" by Donald Knuth, http://pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pdf.Using goto for error handling is a perfectly valid strategy in C, and usually is the best approach. It is used heavily in the linux kernel.Now for C++ on the other hand there might very well be other better approaches, which makes goto not a good choice, my C++ knowledge is too weak tosay anything on that.
hlovdal
A: 

The above comments are all good reasons not to use goto. I can speak from experience that for other programmers who might need to maintain your code goto's make following the logic very hard. I came into a situation of heavy goto spaghetti code and with my OO background it was a nightmare to debug and make changes. Yes this code used goto's for cleanup functions as well. Very frustrating when not needed. Do not use goto's unless absolutely necessary.

Jeff Schmidt
A: 

From all the above comments:

1) goto is very very bad 2) It makes code hard to read and understand. 3) It can cause well known issue "spaghetti code" 4) Everyone agree that it should not be done.

But I am using in following scenerio 1) its used to go forward and only to one label. 2) goto section is used to cleanup code and set return value . If I dont use goto then I need to create a class of every data type. Like I need to wrap int * into a class. 3) Its being followed in whole project.

I agree that its bad but still its making things much easier if followed properly.

Alien01
the int* already exists. It's called std::auto_ptr or boost::shared_ptr.But what you're saying is "it doesn't matter because the entire project already uses gotos", so... why did you ask in the first place?Once again, pick a language. C uses goto, C++ doesn't.
jalf
std::auto_ptr<> is your friend. It is also Exception safe. None of your code is.
Martin York
Easier often does not equate to correct, unfortunately.
PhoenixRedeemer
Going forward means you can bypass the initialization for some symbol. Assuming the compiler lets you do this, something with a constructor would be in an undefined state. My guess is that the only good way to use goto is to exit scopes. You're coding C++, so just do it. Use RAII.
paercebal
A: 

I am not going to say that goto is always bad, but your use of it most certainly is. That kind of "cleanup sections" was pretty common in early 1990's, but using it for new code is pure evil.

Nemanja Trifunovic
I would stay away from words like "pure evil". I would reserve that for things like genocide. We're only programming, after all.
Mike Dunlavey
"Pure evil" is among the most common phrases you can hear in any programming discussion. Other ones are "heresy", "blasphemy", "sin"...
Nemanja Trifunovic
+5  A: 

There are basically two points people are making in regards to gotos and your code:
1. Goto is bad. It's very rare to encounter a place where you need gotos, but I wouldn't suggest striking it completely. Though C++ has smart enough control flow to make goto rarely appropriate.
2. Your mechanism for cleanup is wrong: This point is far more important. In C, using memory management on your own is not only OK but often the best way to do things. In C++, your goal should be to avoid memory management as much as possible. You should avoid memory management as much as possible. Let the compiler do it for you. Rather than using new, just declare variables. The only time you'll really need memory management is when you don't know the size of your data in advance. Even then, you should try to just use some of the STL collections instead.

In the event that you legitimately need memory management (you have not really provided any evidence of this), then you should encapsulate your memory management within a class via constructors to allocate memory and deconstructors to deallocate memory.

Your response that your way of doing things is much easier is not really true in the long run. Firstly, once you get a strong feel for C++ making such constructors will be 2nd nature. Personally, I find using constructors easier than using cleanup code, since I have no need to pay careful attention to make sure I am deallocating properly. Instead, I can just let the object leave scope and the language handles it for me. Also, maintaining them is MUCH easier than maintaining a cleanup section and much less prone to problems.

In short, goto may be a good choice in some situations but not in this one. Here it's just short term laziness.

Brian
+1  A: 

The easiest way to avoid what you are doing here is to put all of this cleanup into some kind of simple structure and create an instance of it. For example instead of:

void MyClass::myFunction()
{
   A* a = new A;
   B* b = new B;
   C* c = new C;
   StartSomeBackgroundTask();
   MaybeBeginAnUndoBlockToo();

   if ( ... )
   {
     goto Exit;
   }

   if ( ... ) { .. }
   else
   {
      ... // what happens if this throws an exception??? too bad...
      goto Exit;
   }

Exit:
  delete a;
  delete b;
  delete c;
  StopMyBackgroundTask();
  EndMyUndoBlock();
}

you should rather do this cleanup in some way like:

struct MyFunctionResourceGuard
{
  MyFunctionResourceGuard( MyClass& owner ) 
  : m_owner( owner )
  , _a( new A )
  , _b( new B )
  , _c( new C )
  {
      m_owner.StartSomeBackgroundTask();
      m_owner.MaybeBeginAnUndoBlockToo();
  }

  ~MyFunctionResourceGuard()
  {
     m_owner.StopMyBackgroundTask();
     m_owner.EndMyUndoBlock();
  }

  std::auto_ptr<A> _a;
  std::auto_ptr<B> _b;
  std::auto_ptr<C> _c;

};

void MyClass::myFunction()
{
   MyFunctionResourceGuard guard( *this );

   if ( ... )
   {
     return;
   }

   if ( ... ) { .. }
   else
   {
      ...
   }
}
Michel
+3  A: 

I think other answers (and their comments) have covered all the important points, but here's one thing that hasn't been done properly yet:

What your code should look like instead:

bool foo() //lowercase bool is a built-in C++ type. Use it if you're writing C++.
{
  try {
    std::auto_ptr<int> p(new int);
    // lots of code, and just return true or false directly when you're done
  }
  catch (std::bad_alloc){ // new throws an exception on OOM, it doesn't return NULL
    cout<<" OOM \n";
    return false;
  }
}

Well, it's shorter, and as far as I can see, more correct (handles the OOM case properly), and most importantly, I didn't need to write any cleanup code or do anything special to "make sure my return value is initialized".

One problem with your code I only really noticed when I wrote this, is "what the hell is bRetVal's value at this point?". I don't know because, it was declared waaaaay above, and it was last assigned to when? At some point above this. I have to read through the entire function to make sure I understand what's going to be returned.

And how do I convince myself that the memory gets freed?

How do I know that we never forget to jump to the cleanup label? I have to work backwards from the cleanup label, finding every goto that points to it, and more importantly, find the ones that aren't there. I need to trace through all paths of the function just to be sure that the function gets cleaned up properly. That reads like spaghetti code to me.

Very fragile code, because every time a resource has to be cleaned up you have to remember to duplicate your cleanup code. Why not write it once, in the type that needs to be cleaned up? And then rely on it being executed automatically, every time we need it?

jalf
+4  A: 

You example is not exception safe.

If you are using goto to clean up the code then if an exception happens before the cleanup code is completely missed. If you claim that you do not use exceptions then you are mistaken because the new will throw bad_alloc when it does not have enough memory.

Also at this point (when bad_alloc is thrown). You stack will be unwound missing all the cleanup code in every function on the way up the call stack thus not cleaning up your code.

You need to look do some research into smart pointer. In the situation above you could just use a std::auto_ptr<>

Also note in C++ code there is no need to check if a pointer is NULL (usually because you never have RAW pointers) but because new will not return NULL (it throws).

Also in C++ unlike (C) it is usually to see early returns in the code. This is because RAII will do the cleanup automatically, while in C code you need to make sure that you add special cleanup code at the end of the function (a bit like your code).

Martin York
A: 

I may have missed something: you jump to the label Exit if P is null, then test to see if it's not null (which it's not) to see if you need to delete it (which isn't necessary because it was never allocated in the first place).

The if/goto won't, and doesn't need to delete p. Replacing the goto with a return false would have the same effect (and then you could remove the Exit label).

The only places I know where goto's are useful are buried deep in nasty parsers (or lexical analyzers), and in faking out state machines (buried in a mass of CPP macros). In those two cases they've been used to make very twisted logic simpler, but that is very rare.

Functions (A calls A'), Try/Catches and setjmp/longjmps are all nicer ways of avoiding a difficult syntax problem.

Paul.

Paul W Homer
A: 

Ignoring the fact that new will never return NULL, take your code:

  BOOL foo()
  {
     BOOL bRetVal = FALSE;

     int *p=NULL;

     p = new int;

     if(p==NULL)
     {
        cout<<" OOM \n";
        goto Exit;
     }

     // Lot of code...

  Exit:
     if(p)
     {
        delete p;
        p= NULL;
     }

     return bRetVal;
  }

and write it like this:

  BOOL foo()
  {
     BOOL bRetVal = FALSE;

     int *p = new int;

     if (p!=NULL)
     {
        // Lot of code...

        delete p;
     }
     else
     {
        cout<<" OOM \n";
     }

     return bRetVal;
  }
jussij
A: 

A lot of people freak out with gotos are evil, they are not. That said, you will never need one, there is just about always a better way.

When I find myself "needing" a goto to do this type of thing, I almost always find that my code is too complex and can be easily broken up into a few method calls that are easier to read and deal with. Your calling code can do something like:

//setup
if(  
     methodA() && 
     methodB() &&
     methodC()
 )
 //cleanup

Not that this is perfect, but it's much easier to follow since all your methods will be named to clearly indicate what the problem might be.

Reading through the comments, however, should indicate that your team has more pressing issues than goto handling.

Bill K
It's a bad programming habbit, there is no need to use them as this will only get you to write longer methods. You can replace the goto in his example by: if (p) delete p; Why add more lines to reach the same?
TomWij
just what I said, thanks for the reinforcement.
Bill K
A: 

That code has a bunch of problems, most of which were pointed out already, eg:

  • The function is too long; refactoring out some code into separate functions might help.

  • Using pointers when normal instances will probably work just fine.

  • Not taking advantage of STL types such as auto_ptr

  • Incorrectly checking for errors, and not catching exceptions. (I would argue that checking for OOM is pointless on the vast majority of platforms, since if you run out of memory you have bigger problems than your software can fix, unless you are writing the OS itself)

I have never needed a goto, and Ive always found that using goto is a symptom of a bigger set of problems. Your case appears to be no exception.

metao
+3  A: 

You should read this thread summary from the linux kernel mailing lists (paying special attention to the responses from Linus Torvalds) before you form a policy for goto:

http://kerneltrap.org/node/553/2131

too much php
But then, Linus' opinions are often quite whacky in themselves. After all, he (co-)invented the flame war.
Konrad Rudolph
Also, C is a bit different than C++. I can see goto being good, in moderation, if used extremely carefully, in some rare circumstances, if you're writing straight C, but if you have the tools of C++ at your disposal, you shouldn't be writing with C idioms.
Adam Jaskiewicz
While Linus is a certifiable nut, he's not wrong about goto statements. When used correctly (as with all things), they are perfectly fine and sometimes preferable to the alternative.
Harvey
A: 

Goto provides better DRY don't-repeat-yourself when "tail-end-logic" is common to some-but-not-all-cases. Especially within a "switch" statement I often use goto's when some of the switch-branches have tail-end-commonality.

switch(){
case a: ... goto L_abTail;
case b: ... goto L_abTail;
L_abTail:
break://end of case b
case c:
.....
}//switch

You have probably noticed than introducing additional curly-braces is enough to satisfy the compiler when you need such tail-end-merging in-the-middle of a routine. In other words you don't need to declare everything way up at the top, that's inferior readability indeed.
...
goto L_skipMiddle;
{
int declInMiddleVar = 0;
.... }
L_skipMiddle: ;
With the later versions of Visual Studio detecting the use of uninitialized variables, I find myself always initializing most variables even though I think they may be assigned in all branches - it's easy to code a "tracing" statement which refs a variable that was never assigned because your mind doesn't think of the tracing statement as "real code" but of course VS will still detect an error. Besides Don't-Repeat-Yourself, assigning label-names to such tail-end-logic even seems to help my mind keep things straight by choosing nice label-names. Without a meaningful label your comments might end up saying the same thing.

OF course if you are actually allocating resources then if auto-ptr doesn't fit you really must use a try-catch, but tail-end-merge-dont-repeat-yourself happens quite often when exception-safety is not an issue.

In summary, while goto can be used to code spaghetti-like structures, in the case of a tail-end-sequence which is common to some-but-not-all-cases then the goto IMPROVES the readability of the code and even maintainability if you would otherwise be copy/pasting stuff so that much later on someone might update one-and-not-the-other. So it's another case where being fanatic about a dogma can be counterproductive.

pngaz
A: 

A few years ago I came up with a pseudo-idiom that avoids goto, and is vaguely similar to doing exception handling in C. It has been probably already invented by someone else so I guess I "discovered it independently" :)

BOOL foo()
{
   BOOL bRetVal = FALSE;
   int *p=NULL;

   do
   {
       p = new int;
       if(p==NULL)
       {
          cout<<" OOM \n";
          break;
       }

       // Lot of code...

       bRetVal = TRUE;

    } while (false);

   if(p)
   {
     delete p;
     p= NULL;
   }

   return bRetVal;
}
ggambett
A: 

The code you're giving us is (almost) C code written inside a C++ file. The kind of memory cleaning you're using would be Ok in a C program not using C++ code/libraries.

In C++, your code is simply unsafe and unreliable. In C++ the kind of management you're asking for is done differently. Use constructors/destructors. Use smart pointers. Use stack. In a word, use RAII.

Your code could (i.e., in C++, SHOULD) be written as:

BOOL foo()
{
   BOOL bRetVal = FALSE;

   std::auto_ptr<int> p = new int;

   // Lot of code...

   return bRetVal ;
}

(note that new-ing a int is somewhat silly in real code, but you can replace int by any kind of object, and then, it makes more sense). Let's imagine we have an object of type T (T could be an int, some C++ class, etc.). Then the code becomes:

BOOL foo()
{
   BOOL bRetVal = FALSE;

   std::auto_ptr<T> p = new T;

   // Lot of code...

   return bRetVal ;
}

or even better, using the stack:

BOOL foo()
{
   BOOL bRetVal = FALSE;

   T p ;

   // Lot of code...

   return bRetVal ;
}

Anyway, any of the above examples are magnitudes more easy to read and secure than your example.

RAII has many facets (i.e. using smart pointers, the stack, using vectors instead of variable length arrays, etc.), but all in all is about writing as little code as possible, letting the compiler clean the stuff at the right moment.

paercebal
A: 

Alien01 wrote: Currently I am working on a project where goto statements are heavely used. The main purpose of goto statements is to have one cleanup section in routine rather than multiple return statements.

I other words, you want to separate the program logic from simple repetitive tedious routines like freeing a resource which might be reserved in different locations of code.

Exception Handling technique is an error handling logic which works in parallel with program logic, it's a more elegant solution since it provides such separation while offering the ability to move control to other blocks of code exactly as the goto statement does, so I modified your script to look like this:

class auxNullPtrException : public std::exception {
public:
    auxNullPtrException::auxNullPtrException()
     : std::exception( " OOM \n") {}
};
BOOL foo()
{
   BOOL bRetVal = FALSE;
   try{ 
       int *p=NULL;
       p = new int;
       if(p==NULL)
       {
      throw auxNullPtrException();
       }
     // Lot of code...
    }
    catch( auxNullPtrException & _auxNullPtrException )
    {
     std::cerr<<_auxNullPtrException.what();
     if(p)
     {
      delete p;
      p= NULL;
     }
    }
   return bRetVal;
}
Josef
A: 

As used in the Linux kernel, goto's used for cleanup work well when a single function must perform 2 or more steps that may need to be undone. Steps need not be memory allocation. It might be a configuration change to a piece of code or in a register of an I/O chipset. Goto's should only be needed in a small number of cases, but often when used correctly, they may be the best solution. They are not evil. They are a tool.

Instead of...

do_step1;
if (failed)
{
  undo_step1;
  return failure;
}

do_step2;
if (failed)
{
  undo_step2;
  undo_step1;
  return failure;
}

do_step3;
if (failed)
{
  undo_step3;
  undo_step2;
  undo_step1;
  return failure;
}

return success;

you can do the same with goto statements like this:

do_step1;
if (failed) goto unwind_step1;

do_step2;
if (failed) goto unwind_step2;

do_step3;
if (failed) goto unwind_step3;

return success;

unwind_step3:
  undo_step3;

unwind_step2:
  undo_step2;

unwind_step1:
  undo_step1;

return failure;

It should be clear that given these two examples, one is preferable to the other. As to the RAII crowd... There is nothing wrong with that approach as long as they can guarantee that the unwinding will always occur in exactly reverse order: 3, 2, 1. And lastly, some people do not use exceptions in their code and instruct the compilers to disable them. Thus not all code must be exception safe.

Harvey
Not sure why this answer was down voted. I'm guessing an anti-goto zealot?
Harvey
Maybe because your code is pure C code, while the question was tagged `C++`? In C++, cleanup code should be done in dtors of local automatic objects.
sbi
eh, that's fair. I'm used to a lot of embedded environments where C++ is often used just as a thin wrapper around C code.
Harvey
+2  A: 

The downside of GOTO is pretty well discussed. I would just add that 1) sometimes you have to use them and should know how to minimize the problems, and 2) some accepted programming techniques are GOTO-in-disguise, so be careful.

1) When you have to use GOTO, such as in ASM or in .bat files, think like a compiler. If you want to code

 if (some_test){
  ... the body ...
}

do what a compiler does. Generate a label whose purpose is to skip over the body, not to do whatever follows. i.e.

 if (not some_test) GOTO label_at_end_of_body
  ... the body ...
label_at_end_of_body:

Not

 if (not some_test) GOTO the_label_named_for_whatever_gets_done_next
  ... the body ...

the_label_named_for_whatever_gets_done_next:

In otherwords, the purpose of the label is not to do something, but to skip over something.

2) What I call GOTO-in-disguise is anything that could be turned into GOTO+LABELS code by just defining a couple macros. An example is the technique of implementing finite-state-automata by having a state variable, and a while-switch statement.

 while (not_done){
    switch(state){
        case S1:
            ... do stuff 1 ...
            state = S2;
            break;
        case S2:
            ... do stuff 2 ...
            state = S1;
            break;
        .........
    }
}

can turn into:

 while (not_done){
    switch(state){
        LABEL(S1):
            ... do stuff 1 ...
            GOTO(S2);
        LABEL(S2):
            ... do stuff 2 ...
            GOTO(S1);
        .........
    }
}

just by defining a couple macros. Just about any FSA can be turned into structured goto-less code. I prefer to stay away from GOTO-in-disguise code because it can get into the same spaghetti-code issues as undisguised gotos.

Added: Just to reassure: I think one mark of a good programmer is recognizing when the common rules don't apply.

Mike Dunlavey
Exactly. Also, I totally agree that good programmers should know when to not apply common rules.
DrJokepu
A: 

In the 8 years I've been programming I've used goto a lot, most of that was in the first year when I was using a version of gwbasic and a book from 1980 that didn't make it clear goto should only be used in certain cases. The only time I've used goto in c++ is when I had code like the following and I'm not sure if there was a better way.

for (int i=0; i

The only situation I know of where goto is still used heavily is mainframe assembly language, and the programmers I know make sure to document where code is jumping and why.

Jared