[C++] Is this a memory leak?

[Arker]

Hello,

I should first mention that although I am not an inexperienced programmer, not much of that experience has been with C++. I'm quite comfortable with C memory management. That may explain why I am confused . Anyways, please consider the following code.

Code: Select all

Object& CreateObject()
{
     Object * obj = new Object(); // created on heap since it will be returned

     return *obj; // is memory leaked here?
}

void UseObject()
{
     // do both of these cause a leak?
     Object obj1 = CreateObject();  // plain old local variable
     Object& obj2 = CreateObject() // local reference
}

To me, this just screams memory leak. But I'm sure I've seen it done before (which means nothing, I know). I've read that you can program in C++ almost entirely without using pointers, and therefore minimizing potential leaks. I'm trying to minimize my use of pointers, hence the CreateObject() function returning a reference.

Anyways, with the number of coders in this forum I thought it would be the perfect place to ask and be sure of the answer .

Thanks,
~djc

[markkuk]

Yes, you are leaking memory. Everything allocated by "new" must be released with "delete", but you make it impossible by returning a reference instead of a pointer. Change your code to

Code: Select all

Object* CreateObject()
{
     Object * obj = new Object(); // created on heap since it will be returned

     return obj; 
} 

and you can delete the returned pointer. Of course wrapping a "new" in a function doesn't make much sense, so simplify your code by replacing calls to "CreateObject()" with the "new" operator.

[_never_]

Well, this doesn't answer your question, but it has been answered anyway. I'm using pointers in C++ very frequently, as they make life easier (linked lists for example). I don't agree with most documentations stating that you should use pointers only if absolutely needed.

My personal rule is to enclose the entire memory handling in some class or a set of functions and test it well. If it works, you won't have to bother with memory handling any more.

By the way... you can't work around pointers if you use dynamic memory allocation (new, delete ...).

[Arker]

Thanks for the replys. I knew it!

Of course wrapping a "new" in a function doesn't make much sense, so simplify your code by replacing calls to "CreateObject()" with the "new" operator.

Yeah I just did that for the purposes of this demo code. I needed to incorporate use of the call stack to demonstrate the leak, but also keep the length of the code minimal so that you would read it .Cheers.

By the way... you can't work around pointers if you use dynamic memory allocation (new, delete ...).

Well that's what I was thinking, and that is what puzzled me about reading the "avoid using pointers" thing. I thought that I understood these C++ references, and apparently I do. The articles I've read that said this (I've seen this several times) made me question though. All but the most trivial programs must make use of the heap at some point, or they'd be a friggin' nightmare to read or code.

Funny how a suggestion intended to prevent memory leaks may inadvertantly cause them. I suppose it depends on how much emphasis the reader puts on the word "avoid".

Thanks for the help,
~djc

[()]

Are you sure it's a problem returning a reference? A reference, as far as I can tell, is basically a constant pointer (in that the contained address cannot be manipulated). For the hell of it I wrote a test program similar to yours where a reference is returned, and freed the object by taking the address of the reference. Valgrind finds no problems with this practice, I tried both new/delete and malloc/free.

Not saying it's a good idea to return a reference to a heap object though, a typical idiom in C++ is to manage new'ed memory with smart pointers so you won't have to worry about deleting it yourself.

[hjnenc]

You can do a

Code: Select all

Object &myobj = CreateObject();
delete &myobj;

but it is awful style. Practically, the first example will therefore lead to a leak since nobody would free the returned reference.

The assignment to obj1 in UseObject leaks memory. There is no way to find out the address of the dynamically allocated object afterwards.

The assignment to obj2 does technically not leak memory, but see above.

To write this without pointers you could do

Code: Select all

Object CreateObject()
{
     Object obj = Object(); // created on stack

     return obj;
}

void UseObject()
{
     // do both of these cause a leak?
     Object obj1 = CreateObject();  // no leak, it's an assignment of objects
     Object& obj2 = CreateObject(); // WRONG the compiler may
                                    // destruct the object after the statement
} 

[Naib]

Nooo!!

Stick with pointers!! they are the most usefull thing in C/C++

Why have you gone to the effort to write a fn that creates an object? I wouldnt class that as such a complex task as to warrent it own fn.

But if that is what you want, why not make a "DestroyObject" function

Now I am more of a C man than C++ and only did a year corse in C++ but cant you do?

Code: Select all

Object *CreateObject()
{
     Object *obj = new Object(); // created on heap since it will be returned
     return obj; // is memory leaked here?
}

void DeleteObject(Object *obj)
{
   delete obj;
}

void UseObject()
{
     // do both of these cause a leak?
     Object *obj1 = CreateObject();  // plain old local variable
     Object& *obj2 = CreateObject() // local reference

  // code using those objects
   DeleteObject(obj1);
   DeleteObject(obj2);


} 

But as I said that just over-bulks the code, just do the new and delete when htey are used

[Bubsy]

There is a nice little template class in the C++ library which handles this very well. It's called auto_ptr:

Code: Select all

auto_ptr<Object> CreateObject()
{
     auto_ptr<Object> obj(new Object()); // Object is still created on the heap

     return obj; // Ownership of Object is transfered to parent function.
}

void UseObject()
{
     auto_ptr<Object> obj1(CreateObject());
     auto_ptr<Object> obj2(CreateObject());

     obj1->fctn();  // an auto_ptr can be used just as a normal pointer

     // Memory is released by destructor of auto_ptr<Object>
}

Have a look at it!

[hjnenc]

Nooo!!

Stick with pointers!! they are the most usefull thing in C/C++

This does not mean you have to use them everywhere

If you have class that represents some value like object (like string, complex number, ...) it does not make sense to use pointers. Its much more natural to treat them like int or double in your program.

Another problem are the exceptions that might be thrown by some code before your DeleteObject function gets called. This would lead to a memory leak. (The auto_ptr template solves this problem - in fact thats one of the main reason why they have been invented). The problem does not appear when you don't use dynamically allocated objects.

Why have you gone to the effort to write a fn that creates an object? I wouldnt class that as such a complex task as to warrent it own fn.

Yeah I just did that for the purposes of this demo code. I needed to incorporate use of the call stack to demonstrate the leak, but also keep the length of the code minimal so that you would read it

[Naib]

note to self:= read entire post!!!!!

fair point. Ill stick with C and VHDL

[_never_]

A reference, as far as I can tell, is basically a constant pointer (in that the contained address cannot be manipulated).

Not really. By convention, a reference should be seen as an "alias". That's also the reason for the requirement, that references must be assigned. If you try to work around pointers using references, you probably end up with much more trouble than you would have with 'real' pointers.

I share Naib's opinion. There is no sense in writing a function to create/delete an object. Use constructors/destructors instead. It's more elegant and allows abstract classes.

[()]

A reference, as far as I can tell, is basically a constant pointer (in that the contained address cannot be manipulated).

Not really. By convention, a reference should be seen as an "alias". That's also the reason for the requirement, that references must be assigned.

Can you tell me the technical difference between a int* const and a int&, apart from the level of indirection introduced by the pointer? The constant pointer has to be initialized like the reference. From a pragmatic point of view the effect is the same, you pass around references to chunks of memory without any copying taking place.

[Viha]

I agree that in C++ the sensible way to implement a factory method like this is to use smart pointers.

Can you tell me the technical difference between a int* const and a int&, apart from the level of indirection introduced by the pointer? The constant pointer has to be initialized like the reference. From a pragmatic point of view the effect is the same, you pass around references to chunks of memory without any copying taking place.

When you pass by reference to a function of yours, you are stating that you expect a valid object (valid in the sense that the object exists). Initializing a reference to an object which you then proceed to delete and then trying to use that reference causes undefined behaviour. A reference can not be initialized with a null "object" either, at least not with defined behaviour. Of course an implementation (of the language) may handle cases of undefined behaviour how it wishes.

[()]

Well, I never said it was a good idea passing (or returning rather) references to heap objects :_) But it's quite possible, that was my initial point I believe.