returning local variables

Question

I came across an issue today regarding local variables. I learned that...

 int * somefunc()
 {
     int x = 5;
     return &x;
 }

 int * y = somefunc();
 //do something

is bad, unsafe, etc. I'd imagine that the case is the same for...

int * somefunc()
{
    int * x = new int;
    x = 5;
    return x;
}

int * y = somefunc();
//do something
delete y;

I've been under the impression for the longest time that this would be safe as the address of x stays in scope when it's returned. However, I'm having second thoughts now and I'm thinking this would lead to memory leaks and other problems, just as the fist example would. Can someone confirm this for me?

Answer 1

As it stands, the second example is wrong. You probably meant this:

int * somefunc()
{
    int * x = new int;
    *x = 5; // note the dereferencing of x here
    return x;
}

Now this is technically fine, but it is prone to errors. First, if after the allocation of x an exception happens, you have to catch it, delete x and then rethrow, or you get a memory-leak. Second, if you return a pointer, the caller has to delete it - callers forget.

The recommended way would be to return a smart pointer, like boost::shared_ptr. This would solve the problems mentioned above. To understand why, read about RAII.

Answer 2

(Edited)

Yes, the second is fine, so long as you dereference that 'x' before assigning!

Answer 3

You might be making int * as just an example, but really, in the case you noted, there is not a reason to return int *, just return int, the actual value is more than good enough. I see these situations all the time, getting overly complicated, when, what is actually needed, is just to simplify.

In the case of 'int *', I can only really think of a realistic case of returning an array of ints, if so, then you need to allocate that, return that, and hopefully, in your documentation, note that it has to be released.

Answer 4

The first approach certainly leads to problems, as you are now well aware.

The second is kind of OK, but demands attention from the programmer because he needs to explicitly delete the returned pointer (as you did). This is harder when your application grows larger, using this method will probably cause problems (memory leaks) as the programmer will find it difficult to keep track of every single variable he needs to deallocate.

A 3rd approach for this scenario, is to pass a variable by reference to be used inside the function, which is way safer.

void somefunc(int& value)
{
    value = 5;
}

// some code that calls somefunc()
int a_value = 0;
somefunc(a_value);
// printing a_value will display 5 

Answer 5

Yes, you're taking the risk of leaking memory. (compile errors aside.) Doing this for an int is silly, but the principle is the same even if it's a large structure.

But understand: you've written C-style code, where you have a function that allocates storage.

If you're trying to learn C++, you should put somefunc() and the data it operates on into a class. Methods and data together. A class can also do RAII as Space_C0wb0y pointed out.

Answer 6

Ok, I would analyze this by answering these questions:

1. What does x contain ? - A memory location(since it is a pointer variable)
2. What is the scope of x? - Since it a a auto variable it's scope is limited to the function somefunc()
3. What happens to auto variables once they exit the local scope ? - They are deleted from the stack space.
4. So what happens to x now after return from somefunc()? - Since it is an auto variable declared on the stack , it's scope(lifetime) is limited to somefunc() and hence will be deleted.
5. Ok so now, what happens to the value pointed to by x? We have a memory leak as the value is allocated on the heap and we have just lost the address when x is deleted.
6. What does y get? - No idea.
7. What happens when y is deleted? - No idea.

Answer 7

The point is not to return a pointer or reference to a local variable, because once the function returns, locals don't exist.

However, the return value still exists, and dynamically allocated memory certainly exists as well.

In C++, we prefer to avoid raw pointers whenever possible. To "return a value that already exists" (i.e. the function does not create a new value), use a reference. To "return a value that didn't already exist" (i.e. the function creates a new value, in the idiomatic sense, not the new keyword sense) use a value, or if necessary, some kind of smart pointer wrapper.

Answer 8

It's both memory leak and a crash (because of the delete).