c++ goto pattern

[hebrerillo]

Hi everyone!

Maybe this is a stupid question. It is related to a pattern that I have come across several times, and I am wondering what would be the best solution. Let's say we have this function:

int myFunction()
{
    MyType* obj1 = new MyType;
    
    MyType* obj2 = new MyType;
    
    if (some_condition)
    {
        //code 
        
        delete obj1;
        delete obj2;
        return 0;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        
        delete obj1;
        delete obj2;
        return 0;
    }
    
    //...
    
    delete obj1;
    delete obj2;
    return 0;

}

As you can see, these three instructions are repeated several times in the function. What would be the best way to group these three instructions? I thought about using 'goto' like this:

int myFunction()
{
    MyType* obj1 = new MyType;
    
    MyType* obj2 = new MyType;
    
    if (some_condition)
    {
        //code 
        
        goto end;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        goto end;        
    }
    
    //...

end:
    
    delete obj1;
    delete obj2;
    return 0;

}

A workmate told me that 'goto' is a bad practice, but did not tell me an alternative. What would be your best solution for this?


Thanks a lot!

[Peter87]

I wouldn't use goto. Instead prefer regular local variables that are automatically destroyed when they go out of scope so that you don't need to worry about using delete.

int myFunction()
{
    MyType obj1;
    MyType obj2;
    
    if (some_condition)
    {
        //code 
        
        return 0;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        
        return 0;
    }
    
    //...
    
    return 0;
}

This has the additional advantage that it will correctly destroy obj1 and obj2 if an exception is thrown.

[hebrerillo]

Hi Peter! Thank you very much for your response!


Those two dynamically allocated variables were just an example. Do not focus on that. Let's say I have three generic instructions that are repeated several times like this:

int myFunction()
{
    
    if (some_condition)
    {
        //code 
        
        instruction1;
        instruction2;
        return 0;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        
        instruction1;
        instruction2;
        return 0;
    }
    
    //...
    
    instruction1;
    instruction2;
    return 0;

}

How would you group these three instructions?

[Peter87]

You could use a function.

void do_instructions()
{
    instruction1;
    instruction2;
}

int myFunction()
{
    if (some_condition)
    {
        //code 
        
        do_instructions();
        return 0;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        
        do_instructions();
        return 0;
    }
    
    //...
    
    do_instructions();
    return 0;
}

[keskiverto]

You could still use a custom local variable to ensure calls on all exits:

struct Foo {
  ~Foo()
  {
    instruction1;
    instruction2;
  }
};

int myFunction()
{
    Foo fubar;
    if (some_condition)
    {
        //code 
        return 0;
    }
    
    //more code...
    
    if (some_condition2)
    {
        //some other code
        return 0;
    }
    //...
    
    return 0;
}

The main question is what data do those instructions require?

There can be other ways to rearrange the code.

int myFunction()
{
    if (some_condition)
    {
        //code 
    }
    else
    {
      //more code...
    
      if (some_condition2)
      {
          //some other code
      }
      else
      {
        //...
      }
    }
    instruction1;
    instruction2;
    return 0;
}

[Repeater]

As keskiverto says, put them in a destructor of an object that goes out of scope. That is the C++ way of doing it. The common example is in resource tidyup; memory, mutex locks, file closing, network disconnections, anything that has to happen when a section of code is left. You create a local object whose destructor takes care of all of that, and you're done.

Then, it doesn't matter how you leave that code; returning from anywhere in it, exception being thrown, someone coming along next year and adding a new return, whatever. When the object goes out of scope, the destructor runs and the necessary functions are called.

The goto: code you showed above is idiomatic C code. It's what people do when they're coding C and need to ensure something happens at the end of a section. The problems they had were exactly what you can predict. As code grew, return paths would be created that didn't go through the necessary closing functions. In C++, exceptions would get thrown so the functions would simply never be called. People would add to the code without knowing or understanding that these functions had to be called, so they wouldn't do it. It's error prone code.

The C++ way is a local object, whose destructor takes care of everything.

As an aside, in the particular example you showed first; the modern C++ way is to not use new. Use a unique pointer and make_unique (or even better stack memory locals rather than dynamic memory objects), and then you can trust the objects will be deleted at exactly the right time without you having to faff around with error prone manual memory management. It's a specific example of what I just talked about; as the unique_pointer goes out of scope, its destructor takes care of everything.

[dhayden]

One way is to use break inside a do ... while (false) construct. When doing this, I always put a comment on the do so that someone reading the code knows it isn't a loop.

int
myFunction()
{
    MyType *obj1 = new MyType;
    MyType *obj2 = new MyType;

    do { // while false
	if (some_condition) {
	    //code 
	    break;
	}
	//more code...
	if (some_condition2) {
	    //some other code
	    break;
	}
    } while (false);
    //...
    delete obj1;
    delete obj2;
    return 0;
}

This has some drawbacks, mostly that the code is fragile: it's easy to break it upon modification. If some future developer adds a return in the middle of the code, the resources will be leaked.

Another option is to allocate/cleanup in one function and do the computations in a helper function. This is usually preferable because the code is less fragile.

static int myFunctionHelper(MyType &obj1, MyType &obj2)
{
    if (some_condition) {
	//code 
	return 0;
    }
    //more code...
    if (some_condition2) {
	//some other code
	return 0;
    }
    return 0;
}
    
  
int
myFunction()
{
    MyType *obj1 = new MyType;
    MyType *obj2 = new MyType;

    int result = myFunctionHelper(*obj1, *obj2);

    delete obj1;
    delete obj2;
    return result;
}

[Duthomhas]

I would like to emphasize what Repeater said. Use RAII to clean up your objects.

As long as things know how to clean up after themselves (RAII), then you can construct them anywhere you need and forget about their memory management — because it has already been properly handled, once, with the definition of the object.

In the case of pointers, use smart pointers.

#include <memory>

int myFunction()
{
  // a RAII pointer...
  auto obj1 = std::make_unique <MyType> ();
  
  // a file...
  std::iostream& f( "some_big_file.txt" );
  
  // anything...
  SomeOtherExpensiveObject q;
  
  if (some_condition)
  {
    // code
    return 1;
  }
  
  // another RAII pointer...
  auto obj2 = std::make_shared <MyType> ( -7 );
  
  if (some_condition2)
  {
    // more code
    return 2;
  }
  
  // alas, code
  
  return 3;
}

tl;dr: Agree with Repeater.

[barnack]

Despite what many can say, gotos are not the evil of this world. A bad goto user is;
Yes, c++ has exceptions. But exceptions add wy more oeverhead than a goto, which is a single machine code instruction. If performance conscerns you, then the goto solution is the way to go. Don't get me wrong, it's easy to use gotos in a bad manner, but what you've shown is an example of a well done goto.


(example about memory allocation)

if(not allocation4 succeeded) goto end4;
if(not allocation3 succeeded) goto end3;
if(not allocation2 succeeded) goto end2;
if(not allocation1 succeeded) goto end1;
do stuff;
end1: deallocate 2
end2: deallocate 3
end3: deallocate 4
end4: //end of method. 

I find this way clearer than a neverending nested if-else chain which would get you to 4 more intendation levels, and better than the flag way to do, which is more memory demanding.
If-else affects readability, one flag per each check affects performance.
Sometimes i see "solutions to not use the infamous goto" which are so bad and useless/unreadable the person who suggested them should get an intensive goto course.
The "add a function" was totally new to me, and even worse. Adding a function means you're making your cpu do 2 context switches per each added method just because you don't want to see a single jump instruction.

So if you see a chance of using a goto, in which it would be more readable and efficient in doing what you need to do, just use it. There's no reason at all not to do so.

EDIT:
Also please, try to rewrite this without gotos and without adding a dozen more variables or functions:

int parse()
{
    Token   tok;

reading:
    tok = gettoken();
    if (tok == END)
        return ACCEPT;
shifting:
    if (shift(tok))
        goto reading;
reducing:
    if (reduce(tok))
        goto shifting;
    return ERROR;
}

Have fun!

[Repeater]

But exceptions add wy more oeverhead than a goto

Unless they never trigger... and if a coder hasn't completely misunderstood the point of them, they should almost never trigger, and if they do trigger (and you're haven't completely misunderstood the point of them) you've got much better things to worry about than a handful of extra cycles.

I find this way clearer than a neverending nested if-else chain
I sure don't. That looks awful. I have to actually read every line to check it's correct. It'd be so easy for someone to copy-paste screw that up, and if they missed one or got one wrong, I wouldn't actually know if it was a mistake, or if someone was being clever in some way.

A memory allocation example to demonstrate good goto? We just covered RAII above. RAII does a far better job. There are good times to use goto, but that's not it.

Adding a function means you're making your cpu do 2 context switches per each added method just because you don't want to see a single jump instruction.

This is, in my opinion, exactly the kind of misguided premature optimisation that causes trouble. A context switch is a thread changeover. Calling a function does not cause a thread changeover. Calling a function is extremely cheap and if you need to worry about the few cycles it costs, the game is already over.

If performance conscerns you, then the goto solution is the way to go.

If performance concerns you, shaving two or three clock cycles off by creating spaghetti code is not the way to go. If performance concerns you, the way to go is to actually measure to see if it's a genuine concern, and if it is, profile your code to identify the places you can make the most savings, and start by examining your algorithms and how you're solving the problems.

[barnack]

predisposing the usage of an exception already means there's more stuff allocated in your memory to handle the eventuality of said exception.
Belive it or not, accept it or not, an exception is and will Always be a complex structure as opposed to the goto which is a single machine code command.

I'm not saying i don't use exceptions. I'm saying "use whatever performs better, and whatever makes what you're going to do more readable. and do so consistently. But don't be scared of a goto just because people started misreading a 1900s man writing"

http://david.tribble.com/text/goto.html

A quote from Dijkstra himself:
"Please don't fall into the trap of believing that I am terribly dogmatical about [the go to statement]. I have the uncomfortable feeling that others are making a religion out of it, as if the conceptual problems of programming could be solved by a single trick, by a simple form of coding discipline!"

Stop hating it, stop using stupid unefficient overhead-intensive workarounds just to avoid a goto.
Use exceptions when you need them, use functions when you need them, use gotos when you need them.
Don't use a function to avoid using a goto, don't use a goto to avoid using an high level cycle.

[Repeater]

Belive it or not, accept it or not, an exception is and will Always be a complex structure as opposed to the goto which is a single machine code command.

I completely believe you. It's a much more complicated structure. The cost of an exception that never fires is zero.

I'm not saying i don't use exceptions.

But you did say that functions calls are expensive. You said to avoid function calls because they're expensive, but they're not expensive. Unless I completely misunderstood what you were talking about with context switches? Did you mean context switch, the expensive thread changing operation, or did you mean something else?

[barnack]

The switch of registers in your cpu. Everytime you call a function the current values in your cpu have to be stored back to the ram, then the cpu loads up totally new values, evaluates the function, then again it goes back to your ram to recover the values you had in your registers before the function was called. Are you comparing that to a jump function?

(i might have used the wrong therm, if it's not "context switch" it's something else. the point remains the same - pls tell me the correct therm)

Also please quote me where i said "You said to avoid function calls because they're expensive". This isn't found anywhere in my messages. I'm saying not to use them as a workaround. Nothing should be used as a workaround when you have a simplier, more strightforward and more readable construct that allows you to achieve the same result.
In the case of the OP code, there's really no reason to add a function which only purpose is use a goto less.

Do you really think a non firing exception costs zero? Please do yourself a favor, write a simple main with an exception which makes sense. Compile it to assembly code. Have fun.

Oh also, can you please redo the last code block i posted with functions? I bet you will really have fun with that!

[barnack]

"If performance concerns you, shaving two or three clock cycles off by creating spaghetti code is not the way to go."

The presence of a goto itself does not imply spaghetti code. The bad usage of a goto does imply spaghtti code the same way the improper usage of any Language sctruture does. Linux's ketnel literally LIVES of gotos.

[Duthomhas]

@barnack
You are either doing a very bad job of arguing your point or you are exposing yourself as exactly the kind of programmer that Dijkstra was trying to reform in his paper.

The entire point of his paper was that structured language provides a better alternative than using a goto, with the added observation that rigid thinking and flimsy excuses make for bad programmers.

Again:
 • structured programming == don't use goto when a better alternative exists
 • bad programmers == inflexible thinking and weak excuses

Let's review:

Exceptions
Exceptions are not gotos. They are a clean way to deal with exceptional == abnormal program failure. They are not appropriate as a replacement for gotos, but their use case overlaps.

(And, believe it or not yourself, exceptions are not so heavy that their use is invalidated. Functions are heavier than gotos. Does your argument extend to say that we should be avoiding functions and other ‘more expensive’, non-machine instruction branch and control constructs as well? Because your final statement certainly seems to say exactly that!)

Performance
“I'm saying 'use whatever performs better...”

Wrong. Code-correctness is more important than performance. And, oddly enough, compilers these days are pretty darn good at making very performant code out of correct code.

What correct code gets you is a lot of very critical bonuses:
 • lower maintenance cost
   • greater readability and comprehension
   • fewer errors introduced at all stages of development
 • better compiler optimizations for language-appropriate design patterns
 • greater usability and interoperability with surrounding code
 • increased encapsulation
 • increased ability to reason about the code
 • less fragile code

Abstractions
It especially kills me when people start spouting premature optimization crap. So goto is a one- or two-instruction opcode. Who the hell cares?

Don’t be an assembler.

It’s a waste of time and it's your compiler's job. The whole purpose of high-level language constructs like functions, objects, exceptions, threads, etc is this: make it easy to write powerful, correct code.

The C++ language makes it possible to write powerful, performant, safe code in just a few lines of what it costs in a language like C (or assembly).

I can easily reason about high-level control structures and complex data in a way that I cannot in C without interleaving and obfuscating the intent of my code with a constant stream of error and safety checks -- safety that is built-in to high-level C++ constructs.

Why on earth would I want to use a goto and throw all that away?

Stop hating poor little goto
None of us here hate goto. I've personally argued for its continued existence and occasional utility. (Though, not to anyone of any real importance, AFAIK.)

OP’s code is an example of bad, dangerous code and an improper use of goto in C++, and a common but still controversial use in C. Anyone sitting in on a code review would probably reject the code outright and tell the programmer to find a better way to manage his resources than with a goto. If a goto did get through code review, it had better have a pretty good justification. (They do exist!)

Examples!
Your first example is a pathetic non-response to the question already answered. In the post immediately above yours. But here you go anyway:

auto ptr1 = make_shared...;  if (!ptr1) return;
auto ptr2 = make_shared...;  if (!ptr2) return;
...
do stuff;
//end of function 

or

auto ptr1 = make_shared...;
auto ptr2 = make_shared...;
...
if (ptr1 and ptr2 ...)
  do stuff;

etc.
Wow, so pretty! So easy. So short and readable. So friggin' unbreakable by an intern's fingers three years from now.

“Also please, try to rewrite this [totally opaque code] without gotos and without adding a dozen more variables or functions:”

Sure. Tell me what the hell it does and I'll gladly tell you how to do it. Until then, it doesn't get into the code base.

Welcome to real-life code review.

[barnack]

"structured language provides a better alternative than using a goto"
this statement would be correct if structured Language and goto usage were mutually exclusive, which they are not.

Cfr: "Structured Programming with go to Statements" of Donald E. Knuth

Maybe someone started missing the context in which Dijkstra's article was published. Maybe someone doesn't know that the title for that article wasn't even proposed by Dijkstra himself.

Again i quote:
"Please don't fall into the trap of believing that I am terribly dogmatical about [the go to statement]. I have the uncomfortable feeling that others are making a religion out of it, as if the conceptual problems of programming could be solved by a single trick, by a simple form of coding discipline!" Dijkstra.

"Use exceptions when you need them, use functions when you need them, use gotos when you need them." Me a couple posts earlier.

That totally opaque code comes from an old parser. A bit of formal languages theory would be enough to know how shift and reduce operate. The point of that opaque code is achieving a flow which would be hardly doable in other manners.

What really makes me upset is using functions to replace a goto in a way they will literally do the exact same thing the goto did, just changing the keyword "goto" with the keyword "return", and over that generating more instructions than required. It is the exact same thing but done worse, over being less clear.

[mbozzi]

@barnack, you're changing the topic.

You argued for OP's code over RAII, because it's simpler and (may) lead to the generation of smaller code.
However:
1. it is not simpler;
2. it does not necessarily lead to the generation of smaller or faster code; and
3. it is extremely error-prone.

Manual resource management (using goto or otherwise) is completely inferior to the RAII idiom. This is obvious from OP's code, which is plainly wrong; new can throw. And even if care is taken to ensure that no exceptions may be thrown, neither OP's code nor your sample releases the resources in the right order.

The sample in your first post in this thread has a typo, too - which can't be understated.

But this fact (that RAII is far superior) doesn't invalidate the use of goto as a control structure. It only invalidates manual resource management, by whatever means that is achieved.

[barnack]

RAII is useful. In some cases i find it a meaningless overhead, expecially if the stated reason is "gotos are the evil on earth".

Right now i can't come up with a simple example of either cases, since all the simple examples that come to my mind are more straightforwardly solved with an if-else. It's in more complex situations that you may need other instruments. If you re-read all my messages without supposing i'm utterly against RAII, you may find that 99% of the time, with the exception of literally a single line, i was speaking quite generally, and the core of everything i said is to use the proper instrument where needed, and to not deny yourself an instrument you may need and go for worse workarounds "just because yeah, let's make our life more complicated than it needs to be". And this applyes to anything really, not only to gotos. It applies to cycles, data structures, pointers, whatever instrument your Language is giving you. You should not be scared of using the right instrument in the right moment.

The sample in your first post in this thread has a typo, too - which can't be understated.

Do you refer to this?

if(not allocation4 succeeded) goto end4;
if(not allocation3 succeeded) goto end3;
if(not allocation2 succeeded) goto end2;
if(not allocation1 succeeded) goto end1;
do stuff;
end1: deallocate 2
end2: deallocate 3
end3: deallocate 4
end4: //end of method.  

Come on, it doesn't take a genious to understand this is pseudocode and not actual c++. Didn't think i had to specify that. Also please tell me what resource is released in the wrong order, i'm pretty curious right now.

[mbozzi]

Do you refer to this?

Yes. You never deallocate 1.

Also please tell me what resource is released in the wrong order

You should release resources in reverse order of their acquisition. This guarantees correct behaviour even when the resources depend on each other.

RAII and goto are orthogonal. RAII has to do with resources. goto doesn't.

The suggestion that OP used goto correctly is wrong; as it stands, OP's code and all the posts here which attempt to manage resources manually contain resource leaks. RAII solves all this at the drop of a hat.

The only thing I have to add further is the following:

It is client-code complexity that we should aim to reduce. For instance: no beginner could write unique_ptr correctly, but as users we don't care what kind of magick the stdlib implements to provide unique_ptr to us, only that unique_ptr allows us to correctly, simply, and safely manage any uniquely-owned pointer-like resource with zero overhead. Properly used, these abstractions make our code simpler.

Clarity and simplicity should be favored over speed and size.

[JLBorges]

> RAII and goto are orthogonal.

Arguably, in a programming language with first class support for RAII, constructs which can be made more elegant, more maintainable, by using goto are a lot fewer.

Some languages without RAII support do provide syntactic sugar;
for example C#'s using keyword (for types which implement the IDisposable interface):

using( var res = new some_resource( /* ... */ ) )
{
     // use the resource 
}